* [PATCH 1/2 v2] KS8695: fix ks8695_rx_irq() bug.
@ 2009-11-16 14:58 zeal
2009-11-12 16:24 ` Figo.zhang
0 siblings, 1 reply; 6+ messages in thread
From: zeal @ 2009-11-16 14:58 UTC (permalink / raw)
To: netdev; +Cc: ben, davem, figo1802, zeal
From: zeal <zealcook@gmail.com>
Sorry for the previously patches. THEY'RE NOT RIGHT. It's my mistake.
Please forgive my noise.
Please review the following patches and ignore the last (v1).
ks8695 rx irq is edge-level. Before arriving at irq handler, the
corresponding status bit has been clear(irq's ack).
So we should not check it after that.
Signed-off-by: zeal <zealcook@gmail.com>
---
drivers/net/arm/ks8695net.c | 22 +++++++---------------
1 files changed, 7 insertions(+), 15 deletions(-)
diff --git a/drivers/net/arm/ks8695net.c b/drivers/net/arm/ks8695net.c
index 0073d19..e15451a 100644
--- a/drivers/net/arm/ks8695net.c
+++ b/drivers/net/arm/ks8695net.c
@@ -433,24 +433,16 @@ ks8695_rx_irq(int irq, void *dev_id)
{
struct net_device *ndev = (struct net_device *)dev_id;
struct ks8695_priv *ksp = netdev_priv(ndev);
- unsigned long status;
-
- unsigned long mask_bit = 1 << ks8695_get_rx_enable_bit(ksp);
spin_lock(&ksp->rx_lock);
- status = readl(KS8695_IRQ_VA + KS8695_INTST);
-
- /*clean rx status bit*/
- writel(status | mask_bit , KS8695_IRQ_VA + KS8695_INTST);
-
- if (status & mask_bit) {
- if (napi_schedule_prep(&ksp->napi)) {
- /*disable rx interrupt*/
- status &= ~mask_bit;
- writel(status , KS8695_IRQ_VA + KS8695_INTEN);
- __napi_schedule(&ksp->napi);
- }
+ if (napi_schedule_prep(&ksp->napi)) {
+ unsigned long status = readl(KS8695_IRQ_VA + KS8695_INTEN);
+ unsigned long mask_bit = 1 << ks8695_get_rx_enable_bit(ksp);
+ /*disable rx interrupt*/
+ status &= ~mask_bit;
+ writel(status , KS8695_IRQ_VA + KS8695_INTEN);
+ __napi_schedule(&ksp->napi);
}
spin_unlock(&ksp->rx_lock);
--
1.5.6.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/2 v2] KS8695: fix ks8695_rx_irq() bug.
2009-11-16 14:58 [PATCH 1/2 v2] KS8695: fix ks8695_rx_irq() bug zeal
@ 2009-11-12 16:24 ` Figo.zhang
2009-11-16 15:18 ` zeal
0 siblings, 1 reply; 6+ messages in thread
From: Figo.zhang @ 2009-11-12 16:24 UTC (permalink / raw)
To: zeal; +Cc: netdev, ben, davem
On Mon, 2009-11-16 at 22:58 +0800, zeal wrote:
> From: zeal <zealcook@gmail.com>
>
> Sorry for the previously patches. THEY'RE NOT RIGHT. It's my mistake.
> Please forgive my noise.
> Please review the following patches and ignore the last (v1).
>
> ks8695 rx irq is edge-level. Before arriving at irq handler, the
> corresponding status bit has been clear(irq's ack).
> So we should not check it after that.
see <KS8695X Integrated Multi-Port Gateway Solution Register
Description> Version 1.00
Interrupt Status Register(INTST Offset 0xE208)
it has said: This edge-triggered interrupt status is cleared by writting
1 , so we should write 1 to clear status bit manually.
>
> Signed-off-by: zeal <zealcook@gmail.com>
> ---
> drivers/net/arm/ks8695net.c | 22 +++++++---------------
> 1 files changed, 7 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/arm/ks8695net.c b/drivers/net/arm/ks8695net.c
> index 0073d19..e15451a 100644
> --- a/drivers/net/arm/ks8695net.c
> +++ b/drivers/net/arm/ks8695net.c
> @@ -433,24 +433,16 @@ ks8695_rx_irq(int irq, void *dev_id)
> {
> struct net_device *ndev = (struct net_device *)dev_id;
> struct ks8695_priv *ksp = netdev_priv(ndev);
> - unsigned long status;
> -
> - unsigned long mask_bit = 1 << ks8695_get_rx_enable_bit(ksp);
>
> spin_lock(&ksp->rx_lock);
>
> - status = readl(KS8695_IRQ_VA + KS8695_INTST);
> -
> - /*clean rx status bit*/
> - writel(status | mask_bit , KS8695_IRQ_VA + KS8695_INTST);
> -
> - if (status & mask_bit) {
> - if (napi_schedule_prep(&ksp->napi)) {
> - /*disable rx interrupt*/
> - status &= ~mask_bit;
> - writel(status , KS8695_IRQ_VA + KS8695_INTEN);
> - __napi_schedule(&ksp->napi);
> - }
> + if (napi_schedule_prep(&ksp->napi)) {
> + unsigned long status = readl(KS8695_IRQ_VA + KS8695_INTEN);
> + unsigned long mask_bit = 1 << ks8695_get_rx_enable_bit(ksp);
> + /*disable rx interrupt*/
> + status &= ~mask_bit;
> + writel(status , KS8695_IRQ_VA + KS8695_INTEN);
> + __napi_schedule(&ksp->napi);
> }
>
> spin_unlock(&ksp->rx_lock);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2 v2] KS8695: fix ks8695_rx_irq() bug.
2009-11-12 16:24 ` Figo.zhang
@ 2009-11-16 15:18 ` zeal
2009-11-12 17:11 ` Figo.zhang
0 siblings, 1 reply; 6+ messages in thread
From: zeal @ 2009-11-16 15:18 UTC (permalink / raw)
To: Figo.zhang; +Cc: netdev, ben, davem
On Fri, Nov 13, 2009 at 12:24 AM, Figo.zhang <figo1802@gmail.com> wrote:
> On Mon, 2009-11-16 at 22:58 +0800, zeal wrote:
>> From: zeal <zealcook@gmail.com>
>>
>> Sorry for the previously patches. THEY'RE NOT RIGHT. It's my mistake.
>> Please forgive my noise.
>> Please review the following patches and ignore the last (v1).
>>
>> ks8695 rx irq is edge-level. Before arriving at irq handler, the
>> corresponding status bit has been clear(irq's ack).
>> So we should not check it after that.
>
> see <KS8695X Integrated Multi-Port Gateway Solution Register
> Description> Version 1.00
>
> Interrupt Status Register(INTST Offset 0xE208)
>
> it has said: This edge-triggered interrupt status is cleared by writting
> 1 , so we should write 1 to clear status bit manually.
>
Yeah, but irq_chip's ack has done that.
Please check arch/arm/mach-ks8695/irq.c ks8695_irq_edge_chip->ack()
>>
>> Signed-off-by: zeal <zealcook@gmail.com>
>> ---
>> drivers/net/arm/ks8695net.c | 22 +++++++---------------
>> 1 files changed, 7 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/net/arm/ks8695net.c b/drivers/net/arm/ks8695net.c
>> index 0073d19..e15451a 100644
>> --- a/drivers/net/arm/ks8695net.c
>> +++ b/drivers/net/arm/ks8695net.c
>> @@ -433,24 +433,16 @@ ks8695_rx_irq(int irq, void *dev_id)
>> {
>> struct net_device *ndev = (struct net_device *)dev_id;
>> struct ks8695_priv *ksp = netdev_priv(ndev);
>> - unsigned long status;
>> -
>> - unsigned long mask_bit = 1 << ks8695_get_rx_enable_bit(ksp);
>>
>> spin_lock(&ksp->rx_lock);
>>
>> - status = readl(KS8695_IRQ_VA + KS8695_INTST);
>> -
>> - /*clean rx status bit*/
>> - writel(status | mask_bit , KS8695_IRQ_VA + KS8695_INTST);
>> -
>> - if (status & mask_bit) {
>> - if (napi_schedule_prep(&ksp->napi)) {
>> - /*disable rx interrupt*/
>> - status &= ~mask_bit;
>> - writel(status , KS8695_IRQ_VA + KS8695_INTEN);
>> - __napi_schedule(&ksp->napi);
>> - }
>> + if (napi_schedule_prep(&ksp->napi)) {
>> + unsigned long status = readl(KS8695_IRQ_VA + KS8695_INTEN);
>> + unsigned long mask_bit = 1 << ks8695_get_rx_enable_bit(ksp);
>> + /*disable rx interrupt*/
>> + status &= ~mask_bit;
>> + writel(status , KS8695_IRQ_VA + KS8695_INTEN);
>> + __napi_schedule(&ksp->napi);
>> }
>>
>> spin_unlock(&ksp->rx_lock);
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Thanks & Regards
zeal
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2 v2] KS8695: fix ks8695_rx_irq() bug.
2009-11-16 15:18 ` zeal
@ 2009-11-12 17:11 ` Figo.zhang
2009-11-17 4:28 ` zeal
0 siblings, 1 reply; 6+ messages in thread
From: Figo.zhang @ 2009-11-12 17:11 UTC (permalink / raw)
To: zeal; +Cc: netdev, ben, davem
On Mon, 2009-11-16 at 23:18 +0800, zeal wrote:
> On Fri, Nov 13, 2009 at 12:24 AM, Figo.zhang <figo1802@gmail.com> wrote:
> > On Mon, 2009-11-16 at 22:58 +0800, zeal wrote:
> >> From: zeal <zealcook@gmail.com>
> >>
> >> Sorry for the previously patches. THEY'RE NOT RIGHT. It's my mistake.
> >> Please forgive my noise.
> >> Please review the following patches and ignore the last (v1).
> >>
> >> ks8695 rx irq is edge-level. Before arriving at irq handler, the
> >> corresponding status bit has been clear(irq's ack).
> >> So we should not check it after that.
> >
> > see <KS8695X Integrated Multi-Port Gateway Solution Register
> > Description> Version 1.00
> >
> > Interrupt Status Register(INTST Offset 0xE208)
> >
> > it has said: This edge-triggered interrupt status is cleared by writting
> > 1 , so we should write 1 to clear status bit manually.
> >
> Yeah, but irq_chip's ack has done that.
> Please check arch/arm/mach-ks8695/irq.c ks8695_irq_edge_chip->ack()
yes, you are right, but you have better add this description at commit
log.
>
> >>
> >> Signed-off-by: zeal <zealcook@gmail.com>
> >> ---
> >> drivers/net/arm/ks8695net.c | 22 +++++++---------------
> >> 1 files changed, 7 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/net/arm/ks8695net.c b/drivers/net/arm/ks8695net.c
> >> index 0073d19..e15451a 100644
> >> --- a/drivers/net/arm/ks8695net.c
> >> +++ b/drivers/net/arm/ks8695net.c
> >> @@ -433,24 +433,16 @@ ks8695_rx_irq(int irq, void *dev_id)
> >> {
> >> struct net_device *ndev = (struct net_device *)dev_id;
> >> struct ks8695_priv *ksp = netdev_priv(ndev);
> >> - unsigned long status;
> >> -
> >> - unsigned long mask_bit = 1 << ks8695_get_rx_enable_bit(ksp);
> >>
> >> spin_lock(&ksp->rx_lock);
> >>
> >> - status = readl(KS8695_IRQ_VA + KS8695_INTST);
> >> -
> >> - /*clean rx status bit*/
> >> - writel(status | mask_bit , KS8695_IRQ_VA + KS8695_INTST);
> >> -
> >> - if (status & mask_bit) {
> >> - if (napi_schedule_prep(&ksp->napi)) {
> >> - /*disable rx interrupt*/
> >> - status &= ~mask_bit;
> >> - writel(status , KS8695_IRQ_VA + KS8695_INTEN);
> >> - __napi_schedule(&ksp->napi);
> >> - }
> >> + if (napi_schedule_prep(&ksp->napi)) {
> >> + unsigned long status = readl(KS8695_IRQ_VA + KS8695_INTEN);
> >> + unsigned long mask_bit = 1 << ks8695_get_rx_enable_bit(ksp);
> >> + /*disable rx interrupt*/
> >> + status &= ~mask_bit;
> >> + writel(status , KS8695_IRQ_VA + KS8695_INTEN);
> >> + __napi_schedule(&ksp->napi);
> >> }
> >>
> >> spin_unlock(&ksp->rx_lock);
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe netdev" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2 v2] KS8695: fix ks8695_rx_irq() bug.
2009-11-12 17:11 ` Figo.zhang
@ 2009-11-17 4:28 ` zeal
2009-11-17 7:51 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: zeal @ 2009-11-17 4:28 UTC (permalink / raw)
To: Figo.zhang; +Cc: netdev, ben, davem
On Fri, Nov 13, 2009 at 1:11 AM, Figo.zhang <figo1802@gmail.com> wrote:
> On Mon, 2009-11-16 at 23:18 +0800, zeal wrote:
>> On Fri, Nov 13, 2009 at 12:24 AM, Figo.zhang <figo1802@gmail.com> wrote:
>> > On Mon, 2009-11-16 at 22:58 +0800, zeal wrote:
>> >> From: zeal <zealcook@gmail.com>
>> >>
>> >> Sorry for the previously patches. THEY'RE NOT RIGHT. It's my mistake.
>> >> Please forgive my noise.
>> >> Please review the following patches and ignore the last (v1).
>> >>
>> >> ks8695 rx irq is edge-level. Before arriving at irq handler, the
>> >> corresponding status bit has been clear(irq's ack).
>> >> So we should not check it after that.
>> >
>> > see <KS8695X Integrated Multi-Port Gateway Solution Register
>> > Description> Version 1.00
>> >
>> > Interrupt Status Register(INTST Offset 0xE208)
>> >
>> > it has said: This edge-triggered interrupt status is cleared by writting
>> > 1 , so we should write 1 to clear status bit manually.
>> >
>> Yeah, but irq_chip's ack has done that.
>> Please check arch/arm/mach-ks8695/irq.c ks8695_irq_edge_chip->ack()
>
> yes, you are right, but you have better add this description at commit
> log.
'the corresponding status bit has been clear(irq's ack)' is not enough?
[snip]
--
Thanks & Regards
zeal
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-11-17 7:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-11-16 14:58 [PATCH 1/2 v2] KS8695: fix ks8695_rx_irq() bug zeal
2009-11-12 16:24 ` Figo.zhang
2009-11-16 15:18 ` zeal
2009-11-12 17:11 ` Figo.zhang
2009-11-17 4:28 ` zeal
2009-11-17 7:51 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).