* Re: [PATCH] staging: qlge: Fix checkpatch errors in the module
2022-02-07 7:15 [PATCH] staging: qlge: Fix checkpatch errors in the module Ayan Choudhary
@ 2022-02-07 7:30 ` Greg KH
2022-02-07 11:03 ` Dan Carpenter
2022-02-07 16:12 ` Randy Dunlap
2 siblings, 0 replies; 4+ messages in thread
From: Greg KH @ 2022-02-07 7:30 UTC (permalink / raw)
To: Ayan Choudhary
Cc: manishc, GR-Linux-NIC-Dev, coiby.xu, netdev, linux-staging,
linux-kernel
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@gmail.com>
> ---
> drivers/staging/qlge/Kconfig | 8 +++++---
> drivers/staging/qlge/TODO | 1 -
> drivers/staging/qlge/qlge.h | 24 ++++++++++++------------
> drivers/staging/qlge/qlge_main.c | 12 +++++++++---
> drivers/staging/qlge/qlge_mpi.c | 11 +++++------
> 5 files changed, 31 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/staging/qlge/Kconfig b/drivers/staging/qlge/Kconfig
> index 6d831ed67965..21fd3f6e33d6 100644
> --- a/drivers/staging/qlge/Kconfig
> +++ b/drivers/staging/qlge/Kconfig
> @@ -5,7 +5,9 @@ config QLGE
> depends on ETHERNET && PCI
> select NET_DEVLINK
> help
> - This driver supports QLogic ISP8XXX 10Gb Ethernet cards.
> + This driver supports QLogic ISP8XXX 10Gb Ethernet cards.
>
> - To compile this driver as a module, choose M here. The module will be
> - called qlge.
> + Say Y here to enable support for QLogic ISP8XXX 10Gb Ethernet cards.
> +
> + To compile this driver as a module, choose M here. The module will be
> + called qlge.
> diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO
> index c76394b9451b..3b57a36d867c 100644
> --- a/drivers/staging/qlge/TODO
> +++ b/drivers/staging/qlge/TODO
> @@ -30,4 +30,3 @@
> * fix weird line wrapping (all over, ex. the ql_set_routing_reg() calls in
> qlge_set_multicast_list()).
> * fix weird indentation (all over, ex. the for loops in qlge_get_stats())
> -* fix checkpatch issues
> 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))
>
> #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)
> #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,
> + 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 */
>
> /* 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 */
>
> 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);
>
> /* 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;
> 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;
> }
>
> return status;
> --
> 2.17.1
>
>
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- Your patch did many different things all at once, making it difficult
to review. All Linux kernel patches need to only do one thing at a
time. If you need to do multiple things (such as clean up all coding
style issues in a file/driver), do it in a sequence of patches, each
one doing only one thing. This will make it easier to review the
patches to ensure that they are correct, and to help alleviate any
merge issues that larger patches can cause.
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.
thanks,
greg k-h's patch email bot
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] staging: qlge: Fix checkpatch errors in the module
2022-02-07 7:15 [PATCH] staging: qlge: Fix checkpatch errors in the module Ayan Choudhary
2022-02-07 7:30 ` Greg KH
@ 2022-02-07 11:03 ` Dan Carpenter
2022-02-07 16:12 ` Randy Dunlap
2 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2022-02-07 11:03 UTC (permalink / raw)
To: Ayan Choudhary
Cc: manishc, GR-Linux-NIC-Dev, coiby.xu, gregkh, netdev,
linux-staging, linux-kernel
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@gmail.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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] staging: qlge: Fix checkpatch errors in the module
2022-02-07 7:15 [PATCH] staging: qlge: Fix checkpatch errors in the module Ayan Choudhary
2022-02-07 7:30 ` Greg KH
2022-02-07 11:03 ` Dan Carpenter
@ 2022-02-07 16:12 ` Randy Dunlap
2 siblings, 0 replies; 4+ messages in thread
From: Randy Dunlap @ 2022-02-07 16:12 UTC (permalink / raw)
To: Ayan Choudhary, manishc, GR-Linux-NIC-Dev, coiby.xu, gregkh
Cc: netdev, linux-staging, linux-kernel
Hi--
On 2/6/22 23:15, 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.
I guess that most of these are warnings and not errors, but since none of
them are quoted here, I cannot tell that for sure.
> Signed-off-by: Ayan Choudhary <ayanchoudhary1025@gmail.com>
> ---
> drivers/staging/qlge/Kconfig | 8 +++++---
> drivers/staging/qlge/TODO | 1 -
> drivers/staging/qlge/qlge.h | 24 ++++++++++++------------
> drivers/staging/qlge/qlge_main.c | 12 +++++++++---
> drivers/staging/qlge/qlge_mpi.c | 11 +++++------
> 5 files changed, 31 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/staging/qlge/Kconfig b/drivers/staging/qlge/Kconfig
> index 6d831ed67965..21fd3f6e33d6 100644
> --- a/drivers/staging/qlge/Kconfig
> +++ b/drivers/staging/qlge/Kconfig
> @@ -5,7 +5,9 @@ config QLGE
> depends on ETHERNET && PCI
> select NET_DEVLINK
> help
> - This driver supports QLogic ISP8XXX 10Gb Ethernet cards.
> + This driver supports QLogic ISP8XXX 10Gb Ethernet cards.
>
> - To compile this driver as a module, choose M here. The module will be
> - called qlge.
> + Say Y here to enable support for QLogic ISP8XXX 10Gb Ethernet cards.
> +
> + To compile this driver as a module, choose M here. The module will be
> + called qlge.
Anyway, please follow coding-style for Kconfig files:
(from Documentation/process/coding-style.rst, section 10):
For all of the Kconfig* configuration files throughout the source tree,
the indentation is somewhat different. Lines under a ``config`` definition
are indented with one tab, while help text is indented an additional two
spaces.
thanks.
--
~Randy
^ permalink raw reply [flat|nested] 4+ messages in thread