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

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

[Lorenzo Bianconi]

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.

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;

---
base-commit: c03a49f3093a4903c8a93c8b5c9a297b5343b169
change-id: 20250422-airoha-en7581-fix-ppe_mbox_data-56df12d4df72

Best regards,
-- 
Lorenzo Bianconi <lorenzo@kernel.org>

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

[Jacob Keller]

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.

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

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?

> 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.

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,

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

[Lorenzo Bianconi]

[-- 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 --]

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

[Jacob Keller]

On 4/23/2025 4:20 AM, Lorenzo Bianconi wrote:

>> 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
> 

Sure. Also, if firmware already has this layout fixed, you could add the
3 padding bytes marked as reserved to make it more obvious they exist
without needing to remember the rules for how the members will align.

Thanks,
Jake

>>
>> 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,
>>

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

[Jakub Kicinski]

On Wed, 23 Apr 2025 09:34:49 -0700 Jacob Keller wrote:
> >> 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.  
> 
> Sure. Also, if firmware already has this layout fixed, you could add the
> 3 padding bytes marked as reserved to make it more obvious they exist
> without needing to remember the rules for how the members will align.

+1 FWIW, mark the padding explicitly is good, but don't make everything
misaligned with __packed