| Message ID | 20250415-airoha-en7581-fix-ppe_mbox_data-v1-1-4408c60ba964@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: airoha: Add missing filed to ppe_mbox_data struct | expand |
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)); }
> 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)); > }
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>
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
> 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 >
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;
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(+) --- base-commit: 23f09f01b495cc510a19b30b6093fb4cb0284aaf change-id: 20250414-airoha-en7581-fix-ppe_mbox_data-2f1880f7486c Best regards,