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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20230913141658.GV401982@kernel.org>
Date:   Wed, 13 Sep 2023 16:16:58 +0200
From:   Simon Horman <horms@...nel.org>
To:     Jinjian Song <songjinjian@...mail.com>
Cc:     davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, corbet@....net, loic.poulain@...aro.org,
        ryazanov.s.a@...il.com, jiri@...nulli.us,
        johannes@...solutions.net, chandrashekar.devegowda@...el.com,
        linuxwwan@...el.com, chiranjeevi.rapolu@...ux.intel.com,
        haijun.liu@...iatek.com, m.chetan.kumar@...ux.intel.com,
        ricardo.martinez@...ux.intel.com, netdev@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        nmarupaka@...gle.com, vsankar@...ovo.com, danielwinkler@...gle.com,
        Jinjian Song <jinjian.song@...ocom.com>
Subject: Re: [net-next v4 4/5] net: wwan: t7xx: Adds sysfs attribute of modem
 event

On Tue, Sep 12, 2023 at 05:48:44PM +0800, Jinjian Song wrote:
> From: Jinjian Song <jinjian.song@...ocom.com>
> 
> 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@linux.intel.com/)
> 
> Signed-off-by: Jinjian Song <jinjian.song@...ocom.com>

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;
>  }

...