From: cyrille.pitchen@atmel.com (Cyrille Pitchen)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 1/4] net/macb: only probe queues once and use stored values
Date: Fri, 27 Mar 2015 17:24:20 +0100 [thread overview]
Message-ID: <55158434.2090708@atmel.com> (raw)
In-Reply-To: <09960ba7389058e026b73409018fe1ee61d6dc57.1427469791.git.nicolas.ferre@atmel.com>
Hi Nicolas,
There is a little mistake in this patch, which will prevent the GEM from
working properly:
Le 27/03/2015 16:34, Nicolas Ferre a ?crit :
> When merging at91_ether and macb driver during 421d9df0628b (net/macb: merge
> at91_ether driver into macb driver) the probe function has been split. The code
> dealing with initialization of queues is now moved in macb_init() which needs
> information computed in the parent macb_probe() function.
> So, add the queue_mask information to the private structure and use it when
> needed in macb_init().
>
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> Cc: Cyrille Pitchen <cyrille.pitchen@atmel.com>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> drivers/net/ethernet/cadence/macb.c | 9 ++++-----
> drivers/net/ethernet/cadence/macb.h | 1 +
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index a0a04b3638e6..b710768172d9 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -2180,7 +2180,7 @@ static void macb_probe_queues(void __iomem *mem,
> static int macb_init(struct platform_device *pdev)
> {
> struct net_device *dev = platform_get_drvdata(pdev);
> - unsigned int hw_q, queue_mask, q, num_queues;
> + unsigned int hw_q, q;
> struct macb *bp = netdev_priv(dev);
> struct macb_queue *queue;
> int err;
> @@ -2226,10 +2226,8 @@ static int macb_init(struct platform_device *pdev)
> * register mapping but we don't want to test the queue index then
> * compute the corresponding register offset at run time.
> */
> - macb_probe_queues(bp->regs, &queue_mask, &num_queues);
> -
> - for (hw_q = 0, q = 0; hw_q < MACB_MAX_QUEUES; ++hw_q) {
> - if (!(queue_mask & (1 << hw_q)))
> + for (hw_q = 0, q = 0; hw_q < bp->num_queues; ++hw_q) {
> + if (!(bp->queue_mask & (1 << hw_q)))
> continue;
The exit condition of the for loop must remain "hw_q < MACB_MAX_QUEUES" and
should not be replaced by "hw_q < bp->num_queues".
Indeed there can be only 2 queues, for instance queue0 and queue7. So if you
change the exit conditon of the loop, queue7 won't be initialized and HRESP
errors will occur.
>
> queue = &bp->queues[q];
> @@ -2715,6 +2713,7 @@ static int macb_probe(struct platform_device *pdev)
> bp->dev = dev;
> bp->regs = mem;
> bp->num_queues = num_queues;
> + bp->queue_mask = queue_mask;
> spin_lock_init(&bp->lock);
>
> platform_set_drvdata(pdev, dev);
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index bc6e35c40822..0b6afac91bfe 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -785,6 +785,7 @@ struct macb {
> size_t rx_buffer_size;
>
> unsigned int num_queues;
> + unsigned int queue_mask;
> struct macb_queue queues[MACB_MAX_QUEUES];
>
> spinlock_t lock;
>
Best Regards,
Cyrille
WARNING: multiple messages have this Message-ID (diff)
From: Cyrille Pitchen <cyrille.pitchen@atmel.com>
To: Nicolas Ferre <nicolas.ferre@atmel.com>, <davem@davemloft.net>,
<netdev@vger.kernel.org>
Cc: <linux-arm-kernel@lists.infradead.org>,
<linux-kernel@vger.kernel.org>,
Boris BREZILLON <boris.brezillon@free-electrons.com>,
<monstr@monstr.eu>, <michal.simek@xilinx.com>,
<punnaia@xilinx.com>
Subject: Re: [PATCH 1/4] net/macb: only probe queues once and use stored values
Date: Fri, 27 Mar 2015 17:24:20 +0100 [thread overview]
Message-ID: <55158434.2090708@atmel.com> (raw)
In-Reply-To: <09960ba7389058e026b73409018fe1ee61d6dc57.1427469791.git.nicolas.ferre@atmel.com>
Hi Nicolas,
There is a little mistake in this patch, which will prevent the GEM from
working properly:
Le 27/03/2015 16:34, Nicolas Ferre a écrit :
> When merging at91_ether and macb driver during 421d9df0628b (net/macb: merge
> at91_ether driver into macb driver) the probe function has been split. The code
> dealing with initialization of queues is now moved in macb_init() which needs
> information computed in the parent macb_probe() function.
> So, add the queue_mask information to the private structure and use it when
> needed in macb_init().
>
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> Cc: Cyrille Pitchen <cyrille.pitchen@atmel.com>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> drivers/net/ethernet/cadence/macb.c | 9 ++++-----
> drivers/net/ethernet/cadence/macb.h | 1 +
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index a0a04b3638e6..b710768172d9 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -2180,7 +2180,7 @@ static void macb_probe_queues(void __iomem *mem,
> static int macb_init(struct platform_device *pdev)
> {
> struct net_device *dev = platform_get_drvdata(pdev);
> - unsigned int hw_q, queue_mask, q, num_queues;
> + unsigned int hw_q, q;
> struct macb *bp = netdev_priv(dev);
> struct macb_queue *queue;
> int err;
> @@ -2226,10 +2226,8 @@ static int macb_init(struct platform_device *pdev)
> * register mapping but we don't want to test the queue index then
> * compute the corresponding register offset at run time.
> */
> - macb_probe_queues(bp->regs, &queue_mask, &num_queues);
> -
> - for (hw_q = 0, q = 0; hw_q < MACB_MAX_QUEUES; ++hw_q) {
> - if (!(queue_mask & (1 << hw_q)))
> + for (hw_q = 0, q = 0; hw_q < bp->num_queues; ++hw_q) {
> + if (!(bp->queue_mask & (1 << hw_q)))
> continue;
The exit condition of the for loop must remain "hw_q < MACB_MAX_QUEUES" and
should not be replaced by "hw_q < bp->num_queues".
Indeed there can be only 2 queues, for instance queue0 and queue7. So if you
change the exit conditon of the loop, queue7 won't be initialized and HRESP
errors will occur.
>
> queue = &bp->queues[q];
> @@ -2715,6 +2713,7 @@ static int macb_probe(struct platform_device *pdev)
> bp->dev = dev;
> bp->regs = mem;
> bp->num_queues = num_queues;
> + bp->queue_mask = queue_mask;
> spin_lock_init(&bp->lock);
>
> platform_set_drvdata(pdev, dev);
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index bc6e35c40822..0b6afac91bfe 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -785,6 +785,7 @@ struct macb {
> size_t rx_buffer_size;
>
> unsigned int num_queues;
> + unsigned int queue_mask;
> struct macb_queue queues[MACB_MAX_QUEUES];
>
> spinlock_t lock;
>
Best Regards,
Cyrille
next prev parent reply other threads:[~2015-03-27 16:24 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-27 15:34 [PATCH 0/4] net/macb: fixes after big driver update Nicolas Ferre
2015-03-27 15:34 ` Nicolas Ferre
2015-03-27 15:34 ` [PATCH 1/4] net/macb: only probe queues once and use stored values Nicolas Ferre
2015-03-27 15:34 ` Nicolas Ferre
2015-03-27 16:24 ` Cyrille Pitchen [this message]
2015-03-27 16:24 ` Cyrille Pitchen
2015-03-30 6:30 ` Nicolas Ferre
2015-03-30 6:30 ` Nicolas Ferre
2015-03-27 22:36 ` Boris Brezillon
2015-03-27 22:36 ` Boris Brezillon
2015-03-27 15:34 ` [PATCH 2/4] net/macb: add comment in macb_probe_queues Nicolas Ferre
2015-03-27 15:34 ` Nicolas Ferre
2015-03-27 15:34 ` [PATCH 3/4] net/macb: fix capabilities configuration Nicolas Ferre
2015-03-27 15:34 ` Nicolas Ferre
2015-03-27 23:02 ` Boris Brezillon
2015-03-27 23:02 ` Boris Brezillon
2015-03-30 6:33 ` Nicolas Ferre
2015-03-30 6:33 ` Nicolas Ferre
2015-03-27 15:34 ` [PATCH 4/4] net/macb: trivial: correct wording of for caps Nicolas Ferre
2015-03-27 15:34 ` Nicolas Ferre
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55158434.2090708@atmel.com \
--to=cyrille.pitchen@atmel.com \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.