| 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
| ||
|
Message-ID: <20220207110318.GG1951@kadam>
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
Powered by blists - more mailing lists