netdev - Re: [PATCH net-next 1/8] net: phy: add a blank line after declarations

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <YMNz3RnzhPak7XIT@lunn.ch>
Date:   Fri, 11 Jun 2021 16:31:57 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Weihang Li <liweihang@...wei.com>
Cc:     davem@...emloft.net, kuba@...nel.org, hkallweit1@...il.com,
        netdev@...r.kernel.org, linuxarm@...wei.com,
        Wenpeng Liang <liangwenpeng@...wei.com>,
        Richard Cochran <richardcochran@...il.com>
Subject: Re: [PATCH net-next 1/8] net: phy: add a blank line after
 declarations

On Fri, Jun 11, 2021 at 02:36:52PM +0800, Weihang Li wrote:
> From: Wenpeng Liang <liangwenpeng@...wei.com>
> 
> There should be a blank line after declarations.
> 
> Cc: Richard Cochran <richardcochran@...il.com>
> Signed-off-by: Wenpeng Liang <liangwenpeng@...wei.com>
> Signed-off-by: Weihang Li <liweihang@...wei.com>
> ---
>  drivers/net/phy/bcm87xx.c  | 4 ++--
>  drivers/net/phy/dp83640.c  | 1 +
>  drivers/net/phy/et1011c.c  | 6 ++++--
>  drivers/net/phy/mdio_bus.c | 1 +
>  drivers/net/phy/qsemi.c    | 1 +
>  5 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/phy/bcm87xx.c b/drivers/net/phy/bcm87xx.c
> index 4ac8fd1..3135634 100644
> --- a/drivers/net/phy/bcm87xx.c
> +++ b/drivers/net/phy/bcm87xx.c
> @@ -54,9 +54,9 @@ static int bcm87xx_of_reg_init(struct phy_device *phydev)
>  		u16 reg		= be32_to_cpup(paddr++);
>  		u16 mask	= be32_to_cpup(paddr++);
>  		u16 val_bits	= be32_to_cpup(paddr++);
> -		int val;
>  		u32 regnum = mdiobus_c45_addr(devid, reg);
> -		val = 0;
> +		int val = 0;
> +

This does a little bit more than add a blank line. Please mention it
in the commit message.

This is to do with trust. If you say you are just added blank lines,
the review can be very quick because you cannot break anything with
just blank lines. But as soon as i see more than just blank lines, i
can no longer trust your description, and i need to look much harder
at your changes.

> --- a/drivers/net/phy/et1011c.c
> +++ b/drivers/net/phy/et1011c.c
> @@ -46,7 +46,8 @@ MODULE_LICENSE("GPL");
>  
>  static int et1011c_config_aneg(struct phy_device *phydev)
>  {
> -	int ctl = 0;
> +	int ctl;
> +
>  	ctl = phy_read(phydev, MII_BMCR);
>  	if (ctl < 0)
>  		return ctl;

Since you made this change, you could go one step further

	int ctl = phy_read(phydev, MII_BMCR);

> @@ -60,9 +61,10 @@ static int et1011c_config_aneg(struct phy_device *phydev)
>  
>  static int et1011c_read_status(struct phy_device *phydev)
>  {
> +	static int speed;
>  	int ret;
>  	u32 val;
> -	static int speed;
> +

This is an O.K. change, but again, more than adding a blank line.

     Andrew