Re: [PATCH] net: ipa: Add IPA_MEM_STATS_DROP_MODEM memory region

[Simon Horman <horms@kernel.org>]

On Thu, Sep 10, 2026 at 03:09:13AM +0000, netdev-bot+sashiko@kernel.org 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