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@xxxxxxxxx> 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