Re: [net-next v4 4/5] net: wwan: t7xx: Adds sysfs attribute of modem event

[Simon Horman]

On Tue, Sep 12, 2023 at 05:48:44PM +0800, Jinjian Song wrote:
> From: Jinjian Song <jinjian.song@xxxxxxxxxxx>
> 
> Adds support for t7xx wwan device firmware flashing & coredump collection
> using devlink.
> 
> Provides sysfs attribute on user space to query the event from modem
> about flashing/coredump/reset.
> 
> Base on the v5 patch version of follow series:
> 'net: wwan: t7xx: fw flashing & coredump support'
> (https://patchwork.kernel.org/project/netdevbpf/patch/fc8bbb0b66a5ff3a489ea9857d79b374508090ef.1674307425.git.m.chetan.kumar@xxxxxxxxxxxxxxx/)
> 
> Signed-off-by: Jinjian Song <jinjian.song@xxxxxxxxxxx>

Hi Jinjian Song,

some minor feedback from my side.

...

> diff --git a/drivers/net/wwan/t7xx/t7xx_pci.c b/drivers/net/wwan/t7xx/t7xx_pci.c

...

> +static ssize_t t7xx_event_show(struct device *dev, struct device_attribute *attr,
> +			       char *buf)
> +{
> +	enum t7xx_event event = T7XX_UNKNOWN;
> +	struct pci_dev *pdev;
> +	struct t7xx_pci_dev *t7xx_dev;

Please arrange local variables in networking code in reverse xmas tree
order - longest line to shortest.

https://github.com/ecree-solarflare/xmastree can be helpful here.

...

> diff --git a/drivers/net/wwan/t7xx/t7xx_port_flash_dump.c b/drivers/net/wwan/t7xx/t7xx_port_flash_dump.c

...

> @@ -361,6 +367,10 @@ static int t7xx_devlink_flash_update(struct devlink *devlink,
>  	clear_bit(T7XX_FLASH_STATUS, &flash_dump->status);
>  
>  err_out:
> +	if (ret)
> +		atomic_set(&port->t7xx_dev->event, T7XX_FLASH_FAILURE);
> +	else
> +		atomic_set(&port->t7xx_dev->event, T7XX_FLASH_SUCCESS);

If the lines immediately above are reached as the result of jumping
to err_out, then port is not initialised.

>  	return ret;
>  }

...