Re: [PATCH net v3] net: airoha: Add missing filed to ppe_mbox_data struct

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3205 bytes --]

> 
> 
> On 4/22/2025 8:59 AM, Lorenzo Bianconi wrote:
> > The official Airoha EN7581 firmware requires adding max_packet filed in
> > ppe_mbox_data struct while the unofficial one used to develop the Airoha
> > EN7581 flowtable support does not require this field.
> > This patch does not introduce any real backwards compatible issue since
> > EN7581 fw is not publicly available in linux-firmware or other
> > repositories (e.g. OpenWrt) yet and the official fw version will use this
> > new layout. For this reason this change needs to be backported.
> > 
> 
> To clarify if I understand correctly:
> 
> The original data structure without max_packet is for an unreleased
> version of firmware which is unofficial and which is not released publicly.

correct

> 
> Then, the official public release will include this additional field,
> and thus won't work with the current kernel code.

correct

> 
> Of course anyone who happens to have the unofficial firmware will need
> to work around this, but that should only include a small handful of
> folks with development images?

yes, right

> 
> > Fixes: 23290c7bc190d ("net: airoha: Introduce Airoha NPU support")
> > Reviewed-by: Simon Horman <horms@kernel.org>
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> > Changes in v3:
> > - resend targeting net tree
> > - Link to v2: https://lore.kernel.org/r/20250417-airoha-en7581-fix-ppe_mbox_data-v2-1-43433cfbe874@kernel.org
> > 
> > Changes in v2:
> > - Add more details to commit log
> > - Link to v1: https://lore.kernel.org/r/20250415-airoha-en7581-fix-ppe_mbox_data-v1-1-4408c60ba964@kernel.org
> > ---
> >  drivers/net/ethernet/airoha/airoha_npu.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/net/ethernet/airoha/airoha_npu.c b/drivers/net/ethernet/airoha/airoha_npu.c
> > index 7a5710f9ccf6a4a4f555ab63d67cb6b318de9b52..16201b5ce9f27866896226c3611b4a154d19bc2c 100644
> > --- a/drivers/net/ethernet/airoha/airoha_npu.c
> > +++ b/drivers/net/ethernet/airoha/airoha_npu.c
> > @@ -104,6 +104,7 @@ struct ppe_mbox_data {
> >  			u8 xpon_hal_api;
> >  			u8 wan_xsi;
> >  			u8 ct_joyme4;
> > +			u8 max_packet;
> >  			int ppe_type;
> >  			int wan_mode;
> >  			int wan_sel;
> > 
> 
> One oddity here is that the structure is not marked __packed. This
> addition of a u8 means there will be a 3-byte gap on platforms which
> have a 4-byte integer... It feels very weird these are ints and not s32
> or something to fully clarify the sizes.

yes, you are right. Let's hold on for a while with this patch and let me ask
Airoha folks if we can "pack" the struct in the NPU firmware binary so we can use
__packed attribute here. In any case I will use "u32" instead of "int" in the next
version.

Regards,
Lorenzo

> 
> Regardless, assuming the correctness that the unofficial firmware is
> only available to developers and isn't widely available:
> 
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> 
> > ---
> > base-commit: c03a49f3093a4903c8a93c8b5c9a297b5343b169
> > change-id: 20250422-airoha-en7581-fix-ppe_mbox_data-56df12d4df72
> > 
> > Best regards,
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]