linux-kernel - Re: [PATCH] staging: qlge: Fix checkpatch errors in the module

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 7 Feb 2022 14:03:18 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Ayan Choudhary <ayanchoudhary1025@...il.com>
Cc:     manishc@...vell.com, GR-Linux-NIC-Dev@...vell.com,
        coiby.xu@...il.com, gregkh@...uxfoundation.org,
        netdev@...r.kernel.org, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: qlge: Fix checkpatch errors in the module

On Sun, Feb 06, 2022 at 11:15:00PM -0800, Ayan Choudhary wrote:
> The qlge module had many checkpatch errors, this patch fixes most of them.
> The errors which presently remain are either false positives or
> introduce unncessary comments in the code.
> 
> Signed-off-by: Ayan Choudhary <ayanchoudhary1025@...il.com>

Your patch does a ton of different stuff and a lot of the changes are
bad.  This patch introduces a bug.  Moves code around for no reason.
Introduces false information into the comments and hurts readability.

> diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
> index 55e0ad759250..7de71bcdb928 100644
> --- a/drivers/staging/qlge/qlge.h
> +++ b/drivers/staging/qlge/qlge.h
> @@ -45,9 +45,8 @@
>  /* Calculate the number of (4k) pages required to
>   * contain a buffer queue of the given length.
>   */
> -#define MAX_DB_PAGES_PER_BQ(x) \
> -		(((x * sizeof(u64)) / DB_PAGE_SIZE) + \
> -		(((x * sizeof(u64)) % DB_PAGE_SIZE) ? 1 : 0))
> +#define MAX_DB_PAGES_PER_BQ(x) ((((x) * sizeof(u64)) / DB_PAGE_SIZE) + \
> +		((((x) * sizeof(u64)) % DB_PAGE_SIZE) ? 1 : 0))

Why did you shift the lines around?  It looks ugly and checkpatch still
complains about side effects of re-using x.

>  
>  #define RX_RING_SHADOW_SPACE	(sizeof(u64) + \
>  		MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN) * sizeof(u64) + \
> @@ -1273,7 +1272,7 @@ struct qlge_net_req_iocb {
>   */
>  struct wqicb {
>  	__le16 len;
> -#define Q_LEN_V		(1 << 4)
> +#define Q_LEN_V		BIT(4)

I'm pretty sure this is deliberate.  LEN suggests length.  It's not a
bit flag.  Anyway, if you're correct please justify your thinking in
the commit messages.

>  #define Q_LEN_CPP_CONT	0x0000
>  #define Q_LEN_CPP_16	0x0001
>  #define Q_LEN_CPP_32	0x0002
> @@ -1308,7 +1307,7 @@ struct cqicb {
>  #define FLAGS_LI	0x40
>  #define FLAGS_LC	0x80
>  	__le16 len;
> -#define LEN_V		(1 << 4)
> +#define LEN_V		BIT(4)
>  #define LEN_CPP_CONT	0x0000
>  #define LEN_CPP_32	0x0001
>  #define LEN_CPP_64	0x0002
> @@ -1365,7 +1364,7 @@ struct tx_ring_desc {
>  	struct tx_ring_desc *next;
>  };
>  
> -#define QL_TXQ_IDX(qdev, skb) (smp_processor_id() % (qdev->tx_ring_count))
> +#define QL_TXQ_IDX(qdev, skb) (smp_processor_id() % ((qdev)->tx_ring_count))
>  
>  struct tx_ring {
>  	/*
> @@ -2030,9 +2029,9 @@ enum {
>  	STS_PAUSE_STD = 0x00000040,
>  	STS_PAUSE_PRI = 0x00000080,
>  	STS_SPEED_MASK = 0x00000038,
> -	STS_SPEED_100Mb = 0x00000000,
> -	STS_SPEED_1Gb = 0x00000008,
> -	STS_SPEED_10Gb = 0x00000010,
> +	STS_SPEED_100MB = 0x00000000,

b stands for bit.  B stands for byte.

> +	STS_SPEED_1GB = 0x00000008,
> +	STS_SPEED_10GB = 0x00000010,
>  	STS_LINK_TYPE_MASK = 0x00000007,
>  	STS_LINK_TYPE_XFI = 0x00000001,
>  	STS_LINK_TYPE_XAUI = 0x00000002,
> @@ -2072,6 +2071,7 @@ struct qlge_adapter *netdev_to_qdev(struct net_device *ndev)
>  
>  	return ndev_priv->qdev;
>  }
> +
>  /*
>   * The main Adapter structure definition.
>   * This structure has all fields relevant to the hardware.
> @@ -2097,8 +2097,8 @@ struct qlge_adapter {
>  	u32 alt_func;		/* PCI function for alternate adapter */
>  	u32 port;		/* Port number this adapter */
>  
> -	spinlock_t adapter_lock;
> -	spinlock_t stats_lock;
> +	spinlock_t adapter_lock; /* Spinlock for adapter */
> +	spinlock_t stats_lock; /* Spinlock for stats */

These comments are not useful.

>  
>  	/* PCI Bus Relative Register Addresses */
>  	void __iomem *reg_base;
> @@ -2116,7 +2116,7 @@ struct qlge_adapter {
>  	u32 mailbox_in;
>  	u32 mailbox_out;
>  	struct mbox_params idc_mbc;
> -	struct mutex	mpi_mutex;
> +	struct mutex	mpi_mutex; /* Mutex for mpi */

Nope.

>  
>  	int tx_ring_size;
>  	int rx_ring_size;
> diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
> index 9873bb2a9ee4..6e4639237334 100644
> --- a/drivers/staging/qlge/qlge_main.c
> +++ b/drivers/staging/qlge/qlge_main.c
> @@ -3890,7 +3890,7 @@ static int qlge_close(struct net_device *ndev)
>  	 * (Rarely happens, but possible.)
>  	 */
>  	while (!test_bit(QL_ADAPTER_UP, &qdev->flags))
> -		msleep(1);
> +		usleep_range(100, 1000);

We don't accept this sort of patch without more testing.

>  
>  	/* Make sure refill_work doesn't re-enable napi */
>  	for (i = 0; i < qdev->rss_ring_count; i++)
> @@ -4085,7 +4085,11 @@ static struct net_device_stats *qlge_get_stats(struct net_device
>  	int i;
>  
>  	/* Get RX stats. */
> -	pkts = mcast = dropped = errors = bytes = 0;
> +	pkts = 0;
> +	mcast = 0;
> +	dropped = 0;
> +	errors = 0;
> +	bytes = 0;

It was basically fine before.  Leave it.

>  	for (i = 0; i < qdev->rss_ring_count; i++, rx_ring++) {
>  		pkts += rx_ring->rx_packets;
>  		bytes += rx_ring->rx_bytes;
> @@ -4100,7 +4104,9 @@ static struct net_device_stats *qlge_get_stats(struct net_device
>  	ndev->stats.multicast = mcast;
>  
>  	/* Get TX stats. */
> -	pkts = errors = bytes = 0;
> +	pkts = 0;
> +	errors = 0;
> +	bytes = 0;
>  	for (i = 0; i < qdev->tx_ring_count; i++, tx_ring++) {
>  		pkts += tx_ring->tx_packets;
>  		bytes += tx_ring->tx_bytes;
> diff --git a/drivers/staging/qlge/qlge_mpi.c b/drivers/staging/qlge/qlge_mpi.c
> index 96a4de6d2b34..6020e337fc0d 100644
> --- a/drivers/staging/qlge/qlge_mpi.c
> +++ b/drivers/staging/qlge/qlge_mpi.c
> @@ -935,13 +935,12 @@ static int qlge_idc_wait(struct qlge_adapter *qdev)
>  			netif_err(qdev, drv, qdev->ndev, "IDC Success.\n");
>  			status = 0;
>  			break;
> -		} else {
> -			netif_err(qdev, drv, qdev->ndev,
> -				  "IDC: Invalid State 0x%.04x.\n",
> -				  mbcp->mbox_out[0]);
> -			status = -EIO;
> -			break;
>  		}
> +		netif_err(qdev, drv, qdev->ndev,
> +			  "IDC: Invalid State 0x%.04x.\n",
> +			  mbcp->mbox_out[0]);
> +		status = -EIO;
> +		break;

No, this breaks the driver.  Please think about what you are doing.

regards,
dan carpenter