On Thu, Sep 10, 2026 at 03:09:13AM +0000, [email protected] wrote: > Thank you for your contribution! Sashiko AI review found 2 potential > issue(s) to consider: > > Critical: 0 · High: 0 · Medium: 1 · Low: 1 > > - [Medium] The newly declared region id `IPA_MEM_MODEM_STATS_DROP` has no > producer and no consumer, so the patch is functionally inert and… > - [Low] Commit-message / code identifier mismatch plus stale peer > documentation for the new drop-statistics region.
Hi Esteban, I'd appreciate it if you could comment on the AI-generated review posted by netdev-bot+sashiko. This is now an expected part of the Netdev development process [1]. Thanks! [1] https://docs.kernel.org/process/maintainer-netdev.html#review-timelines > > -- > > net: ipa: Add IPA_MEM_STATS_DROP_MODEM memory region > > This adds a new ipa_mem_id enumerator for the modem-side drop statistics > region and allows it in ipa_mem_id_valid() only for IPA v5.0 and later. > No platform data file or driver code path references the new region. > > Link: > https://github.com/LineageOS/android_kernel_qcom_sm8450-modules/commit/fc71d0972a2453cc29f873d723a355a902c6d69c > > > diff --git a/drivers/net/ipa/ipa_mem.c b/drivers/net/ipa/ipa_mem.c > > index 078d32a18dbf..3000aaa24530 100644 > > --- a/drivers/net/ipa/ipa_mem.c > > +++ b/drivers/net/ipa/ipa_mem.c > > @@ -166,6 +166,7 @@ static bool ipa_mem_id_valid(struct ipa *ipa, enum > > ipa_mem_id mem_id) > > return false; > > break; > > > > + case IPA_MEM_MODEM_STATS_DROP: > > case IPA_MEM_AP_V4_FILTER: > > case IPA_MEM_AP_V6_FILTER: > > if (version < IPA_VERSION_5_0) > > [Severity: Medium] > Can this new case ever be taken? ipa_mem_id_valid() is only consulted for > ids that already appear in a struct ipa_mem_data.local[] array, and a grep > of the tree at this commit shows IPA_MEM_MODEM_STATS_DROP only in > ipa_mem.h (the declaration) and here. > > The v5.0+ data files that the version >= IPA_VERSION_5_0 gate could apply > to all define just the one drop-stats region: > > drivers/net/ipa/data/ipa_data-v5.0.c > drivers/net/ipa/data/ipa_data-v5.2.c > drivers/net/ipa/data/ipa_data-v5.5.c > .id = IPA_MEM_STATS_DROP, > > and the only consumer of drop-stats memory still looks up the old id: > > drivers/net/ipa/ipa_qmi.c:init_modem_driver_req() { > ... > /* If the DROP stats region is defined, include it */ > mem = ipa_mem_find(ipa, IPA_MEM_STATS_DROP); > if (mem && mem->size) { > req.hw_stats_drop_base_addr_valid = 1; > ... > } > > The commit message says the region "is used in IPA v5.0 and higher", but > with no producer and no consumer the change has no runtime effect. Would it > make sense to land the data-file entry and the QMI consumer in the same > series? > > There also seems to be a layout prerequisite that isn't mentioned. In > ipa_data-v5.0.c, IPA_MEM_STATS_DROP is at offset 0x3870 with size 0x0020 > (ending at 0x3890), and IPA_MEM_MODEM starts at 0x3898 with canary_count 2, > so its two canaries occupy 0x3890-0x3898: > > { > .id = IPA_MEM_STATS_DROP, > .offset = 0x3870, > .size = 0x0020, > .canary_count = 0, > }, > { > .id = IPA_MEM_MODEM, > .offset = 0x3898, > .size = 0x0d48, > .canary_count = 2, > }, > > Doesn't adding a second drop-stats region require shifting IPA_MEM_MODEM and > everything after it? > > > diff --git a/drivers/net/ipa/ipa_mem.h b/drivers/net/ipa/ipa_mem.h > > index b25babade787..56703bada07c 100644 > > --- a/drivers/net/ipa/ipa_mem.h > > +++ b/drivers/net/ipa/ipa_mem.h > > @@ -68,6 +68,7 @@ enum ipa_mem_id { > > IPA_MEM_STATS_QUOTA_AP, /* 0 canaries, optional (IPA v4.0+) */ > > IPA_MEM_STATS_TETHERING, /* 0 canaries, optional (IPA v4.0+) */ > > IPA_MEM_STATS_DROP, /* 0 canaries, optional (IPA v4.0+) */ > > + IPA_MEM_MODEM_STATS_DROP, /* 0 canaries, optional (IPA v5.0+) */ > > [Severity: Low] > The subject and the body both name IPA_MEM_STATS_DROP_MODEM, but the > enumerator added here is IPA_MEM_MODEM_STATS_DROP, and a grep of the tree at > this commit finds no occurrence of IPA_MEM_STATS_DROP_MODEM at all. Could > the changelog and the code be made to agree? > > The neighbouring statistics regions put the owner last: > > IPA_MEM_STATS_QUOTA_MODEM, /* 2/4 canaries (IPA v4.0+) */ > IPA_MEM_STATS_QUOTA_AP, /* 0 canaries, optional (IPA v4.0+) */ > > Would IPA_MEM_STATS_DROP_MODEM (as the subject says) fit that convention > better than IPA_MEM_MODEM_STATS_DROP? > > Also, the comment on the existing IPA_MEM_STATS_DROP still reads "0 > canaries, optional (IPA v4.0+)" with no hint that it becomes the AP-side > region once the modem-side one exists on v5.0+. Could that comment be > updated so a config author can tell which region to use for AP drop > statistics? > > One more thing on the changelog: the paragraph explaining the split ("From > IPA v5.0 onwards, two memory regions for drop stats exist: one for the AP > (that is, the driver) and one for the modem (Q6).") sits below the --- > cut line, so it is not part of the recorded commit message. Was that > intended to be above the cut line? > > -- > Sashiko AI review · > https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909-ipa-modem-drop-v1-1-0c9664f09c4a%40proton.me
