Re: [patch net-next v2 7/7] netdevsim: implement fake flash updating with notifications

[Jiri Pirko <jiri@resnulli.us>]

Wed, May 29, 2019 at 06:47:54PM CEST, jakub.kicinski@netronome.com wrote:
>On Wed, 29 May 2019 10:00:16 +0200, Jiri Pirko wrote:
>> Tue, May 28, 2019 at 10:01:15PM CEST, jakub.kicinski@netronome.com wrote:
>> >On Tue, 28 May 2019 13:48:46 +0200, Jiri Pirko wrote:  
>> >> From: Jiri Pirko <jiri@mellanox.com>
>> >> 
>> >> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> >> ---
>> >> v1->v2:
>> >> - added debugfs toggle to enable/disable flash status notifications  
>> >
>> >Could you please add a selftest making use of netdevsim code?  
>> 
>> How do you imagine the selftest should look like. What should it test
>> exactly?
>
>Well you're adding notifications, probably that the notifications
>arrive correctly?  Plus that devlink doesn't hung when there are no
>notifications.  It doesn't have to be a super advanced test, just
>exercising the code paths in the kernel is fine.
>
>In principle netdevsim is for testing and you add no tests, its not
>the first time you're doing this.

:/
Will add tests and send v3. Monday. Thanks!


>
>> >Sorry, I must have liked the feature so much first time I missed this :)
>> >  
>> >> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
>> >> index b509b941d5ca..c5c417a3c0ce 100644
>> >> --- a/drivers/net/netdevsim/dev.c
>> >> +++ b/drivers/net/netdevsim/dev.c
>> >> @@ -220,8 +222,49 @@ static int nsim_dev_reload(struct devlink *devlink,
>> >>  	return 0;
>> >>  }
>> >>  
>> >> +#define NSIM_DEV_FLASH_SIZE 500000
>> >> +#define NSIM_DEV_FLASH_CHUNK_SIZE 1000
>> >> +#define NSIM_DEV_FLASH_CHUNK_TIME_MS 10
>> >> +
>> >> +static int nsim_dev_flash_update(struct devlink *devlink, const char *file_name,
>> >> +				 const char *component,
>> >> +				 struct netlink_ext_ack *extack)
>> >> +{
>> >> +	struct nsim_dev *nsim_dev = devlink_priv(devlink);
>> >> +	int i;
>> >> +
>> >> +	if (nsim_dev->fw_update_status) {
>> >> +		devlink_flash_update_begin_notify(devlink);
>> >> +		devlink_flash_update_status_notify(devlink,
>> >> +						   "Preparing to flash",
>> >> +						   component, 0, 0);
>> >> +	}
>> >> +
>> >> +	for (i = 0; i < NSIM_DEV_FLASH_SIZE / NSIM_DEV_FLASH_CHUNK_SIZE; i++) {
>> >> +		if (nsim_dev->fw_update_status)
>> >> +			devlink_flash_update_status_notify(devlink, "Flashing",
>> >> +							   component,
>> >> +							   i * NSIM_DEV_FLASH_CHUNK_SIZE,
>> >> +							   NSIM_DEV_FLASH_SIZE);
>> >> +		msleep(NSIM_DEV_FLASH_CHUNK_TIME_MS);  
>> >
>> >In automated testing it may be a little annoying if this takes > 5sec  
>> 
>> I wanted to emulate real device. I can make this 5 sec if you want, no
>> problem.
>
>Is my maths off?  The loop is 5 sec now:
>
> 500000 / 1000 * 10 ms = 5000 ms = 5 sec?

Ah, yes. Originally I had this 20 sec. Pardon me.