[PATCH net] devlink: Remove extra assertion from flash notification logic

[PATCH net] devlink: Remove extra assertion from flash notification logic

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

The mlxsw driver calls to various devlink flash routines even before
users can get any access to the devlink instance itself. For example,
mlxsw_core_fw_rev_validate() one of such functions.

It causes to the WARN_ON to trigger warning about devlink not
registered, while the flow is valid.

Fixes: cf530217408e ("devlink: Notify users when objects are accessible")
Reported-by: Amit Cohen <amcohen@nvidia.com>
Tested-by: Amit Cohen <amcohen@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 net/core/devlink.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 5ba4f9434acd..6face738b16a 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -4229,7 +4229,6 @@ static void __devlink_flash_update_notify(struct devlink *devlink,
 	WARN_ON(cmd != DEVLINK_CMD_FLASH_UPDATE &&
 		cmd != DEVLINK_CMD_FLASH_UPDATE_END &&
 		cmd != DEVLINK_CMD_FLASH_UPDATE_STATUS);
-	WARN_ON(!xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED));
 
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
 	if (!msg)
-- 
2.33.1

Re: [PATCH net] devlink: Remove extra assertion from flash notification logic

[Jakub Kicinski]

On Mon, 15 Nov 2021 20:07:47 +0200 Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> The mlxsw driver calls to various devlink flash routines even before
> users can get any access to the devlink instance itself. For example,
> mlxsw_core_fw_rev_validate() one of such functions.
> 
> It causes to the WARN_ON to trigger warning about devlink not
> registered, while the flow is valid.

So the fix is to remove the warning and keep generating notifications
about objects which to the best understanding of the user space do not
exist?

> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 5ba4f9434acd..6face738b16a 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -4229,7 +4229,6 @@ static void __devlink_flash_update_notify(struct devlink *devlink,
>  	WARN_ON(cmd != DEVLINK_CMD_FLASH_UPDATE &&
>  		cmd != DEVLINK_CMD_FLASH_UPDATE_END &&
>  		cmd != DEVLINK_CMD_FLASH_UPDATE_STATUS);
> -	WARN_ON(!xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED));
>  
>  	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>  	if (!msg)

Re: [PATCH net] devlink: Remove extra assertion from flash notification logic

[Leon Romanovsky]

On Mon, Nov 15, 2021 at 10:14:37AM -0800, Jakub Kicinski wrote:
> On Mon, 15 Nov 2021 20:07:47 +0200 Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > The mlxsw driver calls to various devlink flash routines even before
> > users can get any access to the devlink instance itself. For example,
> > mlxsw_core_fw_rev_validate() one of such functions.
> > 
> > It causes to the WARN_ON to trigger warning about devlink not
> > registered, while the flow is valid.
> 
> So the fix is to remove the warning and keep generating notifications
> about objects which to the best understanding of the user space do not
> exist?

If we delay this mlxsw specific notification, the user will get
DEVLINK_CMD_FLASH_UPDATE and DEVLINK_CMD_FLASH_UPDATE_END at the
same time. I didn't like this, probably users won't like it either,
so decided to go with less invasive solution as possible.

Thanks

> 
> > diff --git a/net/core/devlink.c b/net/core/devlink.c
> > index 5ba4f9434acd..6face738b16a 100644
> > --- a/net/core/devlink.c
> > +++ b/net/core/devlink.c
> > @@ -4229,7 +4229,6 @@ static void __devlink_flash_update_notify(struct devlink *devlink,
> >  	WARN_ON(cmd != DEVLINK_CMD_FLASH_UPDATE &&
> >  		cmd != DEVLINK_CMD_FLASH_UPDATE_END &&
> >  		cmd != DEVLINK_CMD_FLASH_UPDATE_STATUS);
> > -	WARN_ON(!xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED));
> >  
> >  	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> >  	if (!msg)
> 

Re: [PATCH net] devlink: Remove extra assertion from flash notification logic

[Jakub Kicinski]

On Mon, 15 Nov 2021 20:27:35 +0200 Leon Romanovsky wrote:
> On Mon, Nov 15, 2021 at 10:14:37AM -0800, Jakub Kicinski wrote:
> > On Mon, 15 Nov 2021 20:07:47 +0200 Leon Romanovsky wrote:  
> > > From: Leon Romanovsky <leonro@nvidia.com>
> > > 
> > > The mlxsw driver calls to various devlink flash routines even before
> > > users can get any access to the devlink instance itself. For example,
> > > mlxsw_core_fw_rev_validate() one of such functions.
> > > 
> > > It causes to the WARN_ON to trigger warning about devlink not
> > > registered, while the flow is valid.  
> > 
> > So the fix is to remove the warning and keep generating notifications
> > about objects which to the best understanding of the user space do not
> > exist?  
> 
> If we delay this mlxsw specific notification, the user will get
> DEVLINK_CMD_FLASH_UPDATE and DEVLINK_CMD_FLASH_UPDATE_END at the
> same time. I didn't like this, probably users won't like it either,
> so decided to go with less invasive solution as possible.

I'd drop these notifications, the user didn't ask to flash the device,
it's just code reuse in the driver, right?

Re: [PATCH net] devlink: Remove extra assertion from flash notification logic

[Leon Romanovsky]

On Mon, Nov 15, 2021 at 05:15:30PM -0800, Jakub Kicinski wrote:
> On Mon, 15 Nov 2021 20:27:35 +0200 Leon Romanovsky wrote:
> > On Mon, Nov 15, 2021 at 10:14:37AM -0800, Jakub Kicinski wrote:
> > > On Mon, 15 Nov 2021 20:07:47 +0200 Leon Romanovsky wrote:  
> > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > > 
> > > > The mlxsw driver calls to various devlink flash routines even before
> > > > users can get any access to the devlink instance itself. For example,
> > > > mlxsw_core_fw_rev_validate() one of such functions.
> > > > 
> > > > It causes to the WARN_ON to trigger warning about devlink not
> > > > registered, while the flow is valid.  
> > > 
> > > So the fix is to remove the warning and keep generating notifications
> > > about objects which to the best understanding of the user space do not
> > > exist?  
> > 
> > If we delay this mlxsw specific notification, the user will get
> > DEVLINK_CMD_FLASH_UPDATE and DEVLINK_CMD_FLASH_UPDATE_END at the
> > same time. I didn't like this, probably users won't like it either,
> > so decided to go with less invasive solution as possible.
> 
> I'd drop these notifications, the user didn't ask to flash the device,
> it's just code reuse in the driver, right?

Sorry, I missed your reply.

I'm not sure about code reuse, from the code, it looks like attempt to
burn FW during mlxsw register.

__mlxsw_core_bus_device_register
 -> mlxsw_core_fw_rev_validate
  -> mlxsw_core_fw_flash
   -> mlxfw_firmware_flash
    -> mlxfw_status_notify
     -> devlink_flash_update_status_notify
      -> __devlink_flash_update_notify
       -> WARN_ON(...)

The mlxfw_firmware_flash() routine is called by mlx5 too, so I can't
remove mlxfw_status_notify() calls without too much changes.

Easiest solution was to remove WARN_ON(), because no one really
interested in these events anyway. I searched in github and didn't
find any user who listened to them.

Thanks

Re: [PATCH net] devlink: Remove extra assertion from flash notification logic

[Jakub Kicinski]

On Wed, 17 Nov 2021 15:23:18 +0200 Leon Romanovsky wrote:
> > I'd drop these notifications, the user didn't ask to flash the device,
> > it's just code reuse in the driver, right?  
> 
> Sorry, I missed your reply.
> 
> I'm not sure about code reuse, from the code, it looks like attempt to
> burn FW during mlxsw register.
> 
> __mlxsw_core_bus_device_register
>  -> mlxsw_core_fw_rev_validate
>   -> mlxsw_core_fw_flash
>    -> mlxfw_firmware_flash
>     -> mlxfw_status_notify
>      -> devlink_flash_update_status_notify
>       -> __devlink_flash_update_notify
>        -> WARN_ON(...)  
> 
> The mlxfw_firmware_flash() routine is called by mlx5 too, so I can't
> remove mlxfw_status_notify() calls without too much changes.
> 
> Easiest solution was to remove WARN_ON(), because no one really
> interested in these events anyway. I searched in github and didn't
> find any user who listened to them.

Drop in the core. Like this?


 	WARN_ON(cmd != DEVLINK_CMD_FLASH_UPDATE &&
 		cmd != DEVLINK_CMD_FLASH_UPDATE_END &&
 		cmd != DEVLINK_CMD_FLASH_UPDATE_STATUS);
-	WARN_ON(!xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED));
+
+	if (!xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED))
+		return;
 
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
 	if (!msg)

Re: [PATCH net] devlink: Remove extra assertion from flash notification logic

[Leon Romanovsky]

On Wed, Nov 17, 2021 at 06:02:28AM -0800, Jakub Kicinski wrote:
> On Wed, 17 Nov 2021 15:23:18 +0200 Leon Romanovsky wrote:
> > > I'd drop these notifications, the user didn't ask to flash the device,
> > > it's just code reuse in the driver, right?  
> > 
> > Sorry, I missed your reply.
> > 
> > I'm not sure about code reuse, from the code, it looks like attempt to
> > burn FW during mlxsw register.
> > 
> > __mlxsw_core_bus_device_register
> >  -> mlxsw_core_fw_rev_validate
> >   -> mlxsw_core_fw_flash
> >    -> mlxfw_firmware_flash
> >     -> mlxfw_status_notify
> >      -> devlink_flash_update_status_notify
> >       -> __devlink_flash_update_notify
> >        -> WARN_ON(...)  
> > 
> > The mlxfw_firmware_flash() routine is called by mlx5 too, so I can't
> > remove mlxfw_status_notify() calls without too much changes.
> > 
> > Easiest solution was to remove WARN_ON(), because no one really
> > interested in these events anyway. I searched in github and didn't
> > find any user who listened to them.
> 
> Drop in the core. Like this?

Of course, sorry for overlooking of such simple solution.
I'm sending new version right now.

Thanks

> 
> 
>  	WARN_ON(cmd != DEVLINK_CMD_FLASH_UPDATE &&
>  		cmd != DEVLINK_CMD_FLASH_UPDATE_END &&
>  		cmd != DEVLINK_CMD_FLASH_UPDATE_STATUS);
> -	WARN_ON(!xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED));
> +
> +	if (!xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED))
> +		return;
>  
>  	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>  	if (!msg)