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

[PATCH net-next] 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 offload does not require this field. This patch fixes
just a theoretical bug since the Airoha EN7581 firmware is not posted to
linux-firware or other repositories (e.g. OpenWrt) yet.

Fixes: 23290c7bc190d ("net: airoha: Introduce Airoha NPU support")
Signed-off-by: Lorenzo Bianconi <lorenzo@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: 23f09f01b495cc510a19b30b6093fb4cb0284aaf
change-id: 20250414-airoha-en7581-fix-ppe_mbox_data-2f1880f7486c

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

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

[Simon Horman]

On Tue, Apr 15, 2025 at 09:27:21AM +0200, 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 offload does not require this field. This patch fixes
> just a theoretical bug since the Airoha EN7581 firmware is not posted to
> linux-firware or other repositories (e.g. OpenWrt) yet.
> 
> Fixes: 23290c7bc190d ("net: airoha: Introduce Airoha NPU support")
> Signed-off-by: Lorenzo Bianconi <lorenzo@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;

Hi Lorenzo,

I'm a little confused by this.

As I understand it ppe_mbox_data is sent as the data of a mailbox message
send to the device.  But by adding the max_packet field the layout is
changed. The size of the structure changes. And perhaps more importantly
the offset of fields after max_packet, e.g.  wan_mode, change.

Looking at how this is used, f.e. in the following code, I'm unclear on
how this change is backwards compatible.

static int airoha_npu_ppe_init(struct airoha_npu *npu)
{
        struct ppe_mbox_data ppe_data = {
                .func_type = NPU_OP_SET,
                .func_id = PPE_FUNC_SET_WAIT_HWNAT_INIT,
                .init_info = {
                        .ppe_type = PPE_TYPE_L2B_IPV4_IPV6,
                        .wan_mode = QDMA_WAN_ETHER,
                },
        };

        return airoha_npu_send_msg(npu, NPU_FUNC_PPE, &ppe_data,
                                   sizeof(struct ppe_mbox_data));
}

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

[Lorenzo Bianconi]

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

> On Tue, Apr 15, 2025 at 09:27:21AM +0200, 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 offload does not require this field. This patch fixes
> > just a theoretical bug since the Airoha EN7581 firmware is not posted to
> > linux-firware or other repositories (e.g. OpenWrt) yet.
> > 
> > Fixes: 23290c7bc190d ("net: airoha: Introduce Airoha NPU support")
> > Signed-off-by: Lorenzo Bianconi <lorenzo@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;
> 
> Hi Lorenzo,
> 
> I'm a little confused by this.
> 
> As I understand it ppe_mbox_data is sent as the data of a mailbox message
> send to the device.  But by adding the max_packet field the layout is
> changed. The size of the structure changes. And perhaps more importantly
> the offset of fields after max_packet, e.g.  wan_mode, change.
> 
> Looking at how this is used, f.e. in the following code, I'm unclear on
> how this change is backwards compatible.

you are right Simon, this change is not backwards compatible but the fw is
not publicly available yet and the official fw version will use this new layout
(the previous one was just a private version I used to develop the driver).
Can we use this simple approach or do you think we should differentiate the two
firmware version in some way? (even if the previous one will never be used).

Regards,
Lorenzo

> 
> static int airoha_npu_ppe_init(struct airoha_npu *npu)
> {
>         struct ppe_mbox_data ppe_data = {
>                 .func_type = NPU_OP_SET,
>                 .func_id = PPE_FUNC_SET_WAIT_HWNAT_INIT,
>                 .init_info = {
>                         .ppe_type = PPE_TYPE_L2B_IPV4_IPV6,
>                         .wan_mode = QDMA_WAN_ETHER,
>                 },
>         };
> 
>         return airoha_npu_send_msg(npu, NPU_FUNC_PPE, &ppe_data,
>                                    sizeof(struct ppe_mbox_data));
> }

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

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

[Simon Horman]

On Wed, Apr 16, 2025 at 05:55:42PM +0200, Lorenzo Bianconi wrote:
> > On Tue, Apr 15, 2025 at 09:27:21AM +0200, 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 offload does not require this field. This patch fixes
> > > just a theoretical bug since the Airoha EN7581 firmware is not posted to
> > > linux-firware or other repositories (e.g. OpenWrt) yet.
> > > 
> > > Fixes: 23290c7bc190d ("net: airoha: Introduce Airoha NPU support")
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@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;
> > 
> > Hi Lorenzo,
> > 
> > I'm a little confused by this.
> > 
> > As I understand it ppe_mbox_data is sent as the data of a mailbox message
> > send to the device.  But by adding the max_packet field the layout is
> > changed. The size of the structure changes. And perhaps more importantly
> > the offset of fields after max_packet, e.g.  wan_mode, change.
> > 
> > Looking at how this is used, f.e. in the following code, I'm unclear on
> > how this change is backwards compatible.
> 
> you are right Simon, this change is not backwards compatible but the fw
> is not publicly available yet and the official fw version will use this
> new layout (the previous one was just a private version I used to develop
> the driver).  Can we use this simple approach or do you think we should
> differentiate the two firmware version in some way? (even if the previous
> one will never be used).

Hi Lorenzo,

Sorry, I misunderstood the commit message.

Yes, I think this simple approach is fine if there is no
compatibility issue wrt firmwares we can reasonably expect
in the wild. Which seems to be the case here.

Reviewed-by: Simon Horman <horms@kernel.org>

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

[Paolo Abeni]

On 4/16/25 5:55 PM, Lorenzo Bianconi wrote:
>> On Tue, Apr 15, 2025 at 09:27:21AM +0200, 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 offload does not require this field. This patch fixes
>>> just a theoretical bug since the Airoha EN7581 firmware is not posted to
>>> linux-firware or other repositories (e.g. OpenWrt) yet.
>>>
>>> Fixes: 23290c7bc190d ("net: airoha: Introduce Airoha NPU support")
>>> Signed-off-by: Lorenzo Bianconi <lorenzo@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;
>>
>> Hi Lorenzo,
>>
>> I'm a little confused by this.
>>
>> As I understand it ppe_mbox_data is sent as the data of a mailbox message
>> send to the device.  But by adding the max_packet field the layout is
>> changed. The size of the structure changes. And perhaps more importantly
>> the offset of fields after max_packet, e.g.  wan_mode, change.
>>
>> Looking at how this is used, f.e. in the following code, I'm unclear on
>> how this change is backwards compatible.
> 
> you are right Simon, this change is not backwards compatible but the fw is
> not publicly available yet and the official fw version will use this new layout
> (the previous one was just a private version I used to develop the driver).
> Can we use this simple approach or do you think we should differentiate the two
> firmware version in some way? (even if the previous one will never be used).

I think it's better if you clarify the commit message. I read the above
as the current (unpatched) code will not work with the official
firmware, so bug addressed here does not look theoretical.

Thanks,

Paolo

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

[Lorenzo Bianconi]

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

> On 4/16/25 5:55 PM, Lorenzo Bianconi wrote:
> >> On Tue, Apr 15, 2025 at 09:27:21AM +0200, 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 offload does not require this field. This patch fixes
> >>> just a theoretical bug since the Airoha EN7581 firmware is not posted to
> >>> linux-firware or other repositories (e.g. OpenWrt) yet.
> >>>
> >>> Fixes: 23290c7bc190d ("net: airoha: Introduce Airoha NPU support")
> >>> Signed-off-by: Lorenzo Bianconi <lorenzo@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;
> >>
> >> Hi Lorenzo,
> >>
> >> I'm a little confused by this.
> >>
> >> As I understand it ppe_mbox_data is sent as the data of a mailbox message
> >> send to the device.  But by adding the max_packet field the layout is
> >> changed. The size of the structure changes. And perhaps more importantly
> >> the offset of fields after max_packet, e.g.  wan_mode, change.
> >>
> >> Looking at how this is used, f.e. in the following code, I'm unclear on
> >> how this change is backwards compatible.
> > 
> > you are right Simon, this change is not backwards compatible but the fw is
> > not publicly available yet and the official fw version will use this new layout
> > (the previous one was just a private version I used to develop the driver).
> > Can we use this simple approach or do you think we should differentiate the two
> > firmware version in some way? (even if the previous one will never be used).
> 
> I think it's better if you clarify the commit message. I read the above
> as the current (unpatched) code will not work with the official
> firmware, so bug addressed here does not look theoretical.

ack, I will fix it in v2.

Regards,
Lorenzo

> 
> Thanks,
> 
> Paolo
> 

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