2008-10-06 17:27:14

by Anton Vorontsov

[permalink] [raw]
Subject: [PATCH] pata_of_platform: fix no irq handling

When no irq specified the pata_of_platform fills the irq_res with -1,
which is wrong to do for two reasons:

1. By definition, 'no irq' should be IRQ 0, not some negative integer;
2. pata_platform checks for irq_res.start > 0, but since irq_res.start
is unsigned type, the check will be true for `-1'.

Reported-by: Steven A. Falco <[email protected]>
Signed-off-by: Anton Vorontsov <[email protected]>
---

Resending again...

drivers/ata/pata_of_platform.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
index 408da30..1f18ad9 100644
--- a/drivers/ata/pata_of_platform.c
+++ b/drivers/ata/pata_of_platform.c
@@ -52,7 +52,7 @@ static int __devinit pata_of_platform_probe(struct of_device *ofdev,

ret = of_irq_to_resource(dn, 0, &irq_res);
if (ret == NO_IRQ)
- irq_res.start = irq_res.end = -1;
+ irq_res.start = irq_res.end = 0;
else
irq_res.flags = 0;

--
1.5.6.3


2008-10-06 20:41:32

by Matt Sealey

[permalink] [raw]
Subject: Re: [PATCH] pata_of_platform: fix no irq handling

There is a simple problem with the patch which is that an "IRQ 0" can and does
actually exist on a bunch of platforms, at least to the best of my knowledge.

Checking for -1 (which means for definite, no irq at all, because it is
totally unambiguous, as a -1 IRQ numbering is "impossible") is more correct.

The problem is the check against an unsigned value for interrupts (is there
any reason why you would need 4 billion interrupts possible instead of just
2 billion?) although I must say, the patch will work, and probably 99.9999999%
of people will never see a problem with it :)

--
Matt Sealey <[email protected]>
Genesi, Manager, Developer Relations

Anton Vorontsov wrote:
> When no irq specified the pata_of_platform fills the irq_res with -1,
> which is wrong to do for two reasons:
>
> 1. By definition, 'no irq' should be IRQ 0, not some negative integer;
> 2. pata_platform checks for irq_res.start > 0, but since irq_res.start
> is unsigned type, the check will be true for `-1'.
>
> Reported-by: Steven A. Falco <[email protected]>
> Signed-off-by: Anton Vorontsov <[email protected]>
> ---
>
> Resending again...
>
> drivers/ata/pata_of_platform.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
> index 408da30..1f18ad9 100644
> --- a/drivers/ata/pata_of_platform.c
> +++ b/drivers/ata/pata_of_platform.c
> @@ -52,7 +52,7 @@ static int __devinit pata_of_platform_probe(struct of_device *ofdev,
>
> ret = of_irq_to_resource(dn, 0, &irq_res);
> if (ret == NO_IRQ)
> - irq_res.start = irq_res.end = -1;
> + irq_res.start = irq_res.end = 0;
> else
> irq_res.flags = 0;
>

2008-10-06 21:32:20

by Anton Vorontsov

[permalink] [raw]
Subject: Re: [PATCH] pata_of_platform: fix no irq handling

On Mon, Oct 06, 2008 at 03:41:19PM -0500, Matt Sealey wrote:
> There is a simple problem with the patch which is that an "IRQ 0" can and does
> actually exist on a bunch of platforms, at least to the best of my knowledge.
>
> Checking for -1 (which means for definite, no irq at all, because it is
> totally unambiguous, as a -1 IRQ numbering is "impossible") is more correct.

This was discussed years ago.

http://lkml.org/lkml/2005/11/22/159
http://lkml.org/lkml/2005/11/22/227

--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2

2008-10-07 01:33:21

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH] pata_of_platform: fix no irq handling

Anton Vorontsov wrote:
> On Mon, Oct 06, 2008 at 03:41:19PM -0500, Matt Sealey wrote:
>> There is a simple problem with the patch which is that an "IRQ 0" can and does
>> actually exist on a bunch of platforms, at least to the best of my knowledge.
>>
>> Checking for -1 (which means for definite, no irq at all, because it is
>> totally unambiguous, as a -1 IRQ numbering is "impossible") is more correct.
>
> This was discussed years ago.
>
> http://lkml.org/lkml/2005/11/22/159
> http://lkml.org/lkml/2005/11/22/227
>

Would this break any existing platforms? If so, can those be fixed
together or does it become a much bigger problem that way?

Thanks.

--
tejun

2008-10-07 09:26:22

by Anton Vorontsov

[permalink] [raw]
Subject: Re: [PATCH] pata_of_platform: fix no irq handling

On Tue, Oct 07, 2008 at 10:30:59AM +0900, Tejun Heo wrote:
> Anton Vorontsov wrote:
> > On Mon, Oct 06, 2008 at 03:41:19PM -0500, Matt Sealey wrote:
> >> There is a simple problem with the patch which is that an "IRQ 0" can and does
> >> actually exist on a bunch of platforms, at least to the best of my knowledge.
> >>
> >> Checking for -1 (which means for definite, no irq at all, because it is
> >> totally unambiguous, as a -1 IRQ numbering is "impossible") is more correct.
> >
> > This was discussed years ago.
> >
> > http://lkml.org/lkml/2005/11/22/159
> > http://lkml.org/lkml/2005/11/22/227
> >
>
> Would this break any existing platforms?

Nope.

The driver is only available for PPC platforms.

As time goes by one can change `depend on PPC_OF' to just `depends on
OF', so that the driver will be also available for SPARC. And still
it will work, because SPARC also understands VIRQ0 as invalid VIRQ.


Thanks,

--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2

2008-10-07 09:43:26

by Wang Jian

[permalink] [raw]
Subject: Re: [PATCH] pata_of_platform: fix no irq handling

Tejun Heo wrote:
> Anton Vorontsov wrote:
>> On Mon, Oct 06, 2008 at 03:41:19PM -0500, Matt Sealey wrote:
>>> There is a simple problem with the patch which is that an "IRQ 0" can and does
>>> actually exist on a bunch of platforms, at least to the best of my knowledge.
>>>
>>> Checking for -1 (which means for definite, no irq at all, because it is
>>> totally unambiguous, as a -1 IRQ numbering is "impossible") is more correct.
>> This was discussed years ago.
>>
>> http://lkml.org/lkml/2005/11/22/159
>> http://lkml.org/lkml/2005/11/22/227
>>
>
> Would this break any existing platforms? If so, can those be fixed
> together or does it become a much bigger problem that way?
>

Pata_of_platform stacks upon pata_platform. This patch fixes problem
concerning definition of "no irq" without touch any other place. So
far I can't see any new problem.

2008-10-07 09:51:32

by Alan

[permalink] [raw]
Subject: Re: [PATCH] pata_of_platform: fix no irq handling

> > This was discussed years ago.
> >
> > http://lkml.org/lkml/2005/11/22/159
> > http://lkml.org/lkml/2005/11/22/227
> >
>
> Would this break any existing platforms? If so, can those be fixed
> together or does it become a much bigger problem that way?

Zero means no IRQ. Any platform with bits of code left over exposing IRQ
0 is already not supported by lots of driver code including libata.

As IRQs are unsigned using -1 is asking for trouble

2008-10-07 10:16:14

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [PATCH] pata_of_platform: fix no irq handling

On Tue, 2008-10-07 at 13:26 +0400, Anton Vorontsov wrote:
> On Tue, Oct 07, 2008 at 10:30:59AM +0900, Tejun Heo wrote:
> > Anton Vorontsov wrote:
> > > On Mon, Oct 06, 2008 at 03:41:19PM -0500, Matt Sealey wrote:
> > >> There is a simple problem with the patch which is that an "IRQ 0" can and does
> > >> actually exist on a bunch of platforms, at least to the best of my knowledge.
> > >>
> > >> Checking for -1 (which means for definite, no irq at all, because it is
> > >> totally unambiguous, as a -1 IRQ numbering is "impossible") is more correct.
> > >
> > > This was discussed years ago.
> > >
> > > http://lkml.org/lkml/2005/11/22/159
> > > http://lkml.org/lkml/2005/11/22/227
> > >
> >
> > Would this break any existing platforms?
>
> Nope.
>
> The driver is only available for PPC platforms.
>
> As time goes by one can change `depend on PPC_OF' to just `depends on
> OF', so that the driver will be also available for SPARC. And still
> it will work, because SPARC also understands VIRQ0 as invalid VIRQ.
>

Yup, I agree. I'll pick it up in my next batch.

Cheers,
Ben.

2008-10-08 08:41:27

by David Woodhouse

[permalink] [raw]
Subject: Re: [PATCH] pata_of_platform: fix no irq handling

On Tue, 2008-10-07 at 10:37 +0100, Alan Cox wrote:
> Zero means no IRQ. Any platform with bits of code left over exposing IRQ
> 0 is already not supported by lots of driver code including libata.

...and must implement some kind of interrupt remapping crap just to work
around this bogus design decision.

--
David Woodhouse Open Source Technology Centre
[email protected] Intel Corporation

2008-10-08 09:04:48

by Alan

[permalink] [raw]
Subject: Re: [PATCH] pata_of_platform: fix no irq handling

On Wed, 08 Oct 2008 09:40:54 +0100
David Woodhouse <[email protected]> wrote:

> On Tue, 2008-10-07 at 10:37 +0100, Alan Cox wrote:
> > Zero means no IRQ. Any platform with bits of code left over exposing IRQ
> > 0 is already not supported by lots of driver code including libata.
>
> ...and must implement some kind of interrupt remapping crap just to work
> around this bogus design decision.

I'll leave you to argue with Linus about that, but since that was the
decision back in 2005 (for good C reasons) we can safely rely on it.

2008-10-08 10:00:20

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH] pata_of_platform: fix no irq handling

On Wed, 8 Oct 2008, Alan Cox wrote:
> On Wed, 08 Oct 2008 09:40:54 +0100
> David Woodhouse <[email protected]> wrote:
>
> > On Tue, 2008-10-07 at 10:37 +0100, Alan Cox wrote:
> > > Zero means no IRQ. Any platform with bits of code left over exposing IRQ
> > > 0 is already not supported by lots of driver code including libata.
> >
> > ...and must implement some kind of interrupt remapping crap just to work
> > around this bogus design decision.
>
> I'll leave you to argue with Linus about that, but since that was the
> decision back in 2005 (for good C reasons) we can safely rely on it.

`git grep NO_IRQ include arch/*/include' is still quite enlightening...

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: [email protected]
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010

2008-10-08 10:36:58

by Alan

[permalink] [raw]
Subject: Re: [PATCH] pata_of_platform: fix no irq handling

> > I'll leave you to argue with Linus about that, but since that was the
> > decision back in 2005 (for good C reasons) we can safely rely on it.
>
> `git grep NO_IRQ include arch/*/include' is still quite enlightening...

Good guide to platform code we should delete really

2008-10-10 17:55:50

by Paul Mundt

[permalink] [raw]
Subject: Re: [PATCH] pata_of_platform: fix no irq handling

On Wed, Oct 08, 2008 at 11:59:59AM +0200, Geert Uytterhoeven wrote:
> On Wed, 8 Oct 2008, Alan Cox wrote:
> > On Wed, 08 Oct 2008 09:40:54 +0100
> > David Woodhouse <[email protected]> wrote:
> >
> > > On Tue, 2008-10-07 at 10:37 +0100, Alan Cox wrote:
> > > > Zero means no IRQ. Any platform with bits of code left over exposing IRQ
> > > > 0 is already not supported by lots of driver code including libata.
> > >
> > > ...and must implement some kind of interrupt remapping crap just to work
> > > around this bogus design decision.
> >
> > I'll leave you to argue with Linus about that, but since that was the
> > decision back in 2005 (for good C reasons) we can safely rely on it.
>
> `git grep NO_IRQ include arch/*/include' is still quite enlightening...
>
In the case of PCI where IRQ is unsigned int, that's certainly bogus. The
problem originates on platforms where a 1:1 mapping between vector and
IRQ exists, where 0 is a valid value. This then needs to be remapped in
to an IRQ cookie that has a non-0 value in order to be assigned to
dev->irq. Despite whether this is bogus or not, there's not much to be
done about it. Those of us with vectored IRQ platforms generally have
enough sources that the use of IRQ-0 doesn't matter, and it's not worth
the headache of setting up the remapping crap.

As an example, on SH, IRQ-0 is mapped to vector 0x200. It is '0' for
symbolic reasons only. It's just as easy to pad out irq_desc and treat it
as an off-by-1 to work around all of code that assumes NO_IRQ == 0. There
is enough code in the kernel today that makes the non-zero IRQ cookie
assumption that platforms that do otherwise are simply broken.

2008-10-13 06:59:16

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH] pata_of_platform: fix no irq handling

Anton Vorontsov wrote:
> When no irq specified the pata_of_platform fills the irq_res with -1,
> which is wrong to do for two reasons:
>
> 1. By definition, 'no irq' should be IRQ 0, not some negative integer;
> 2. pata_platform checks for irq_res.start > 0, but since irq_res.start
> is unsigned type, the check will be true for `-1'.
>
> Reported-by: Steven A. Falco <[email protected]>
> Signed-off-by: Anton Vorontsov <[email protected]>

applied to #tj-upstream (will soon be sent upstream)

--
tejun

2008-10-13 13:27:35

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] pata_of_platform: fix no irq handling

Tejun Heo wrote:
> Anton Vorontsov wrote:
>> When no irq specified the pata_of_platform fills the irq_res with -1,
>> which is wrong to do for two reasons:
>>
>> 1. By definition, 'no irq' should be IRQ 0, not some negative integer;
>> 2. pata_platform checks for irq_res.start > 0, but since irq_res.start
>> is unsigned type, the check will be true for `-1'.
>>
>> Reported-by: Steven A. Falco <[email protected]>
>> Signed-off-by: Anton Vorontsov <[email protected]>
>
> applied to #tj-upstream (will soon be sent upstream)

I have returned! muhahahahahaha....

2008-10-13 14:00:12

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH] pata_of_platform: fix no irq handling

Jeff Garzik wrote:
> Tejun Heo wrote:
>> Anton Vorontsov wrote:
>>> When no irq specified the pata_of_platform fills the irq_res with -1,
>>> which is wrong to do for two reasons:
>>>
>>> 1. By definition, 'no irq' should be IRQ 0, not some negative integer;
>>> 2. pata_platform checks for irq_res.start > 0, but since irq_res.start
>>> is unsigned type, the check will be true for `-1'.
>>>
>>> Reported-by: Steven A. Falco <[email protected]>
>>> Signed-off-by: Anton Vorontsov <[email protected]>
>>
>> applied to #tj-upstream (will soon be sent upstream)
>
> I have returned! muhahahahahaha....

Ah.. great. Hope you enjoyed the vacation. Can you please pull from
#tj-upstream?

git://git.kernel.org/pub/scm/linux/kernel/git/tj/libata-dev.git tj-upstream
http://git.kernel.org/?p=linux/kernel/git/tj/libata-dev.git;a=shortlog;h=tj-upstream

Nothing much. Rafael's skip-spin-off patchset + Elias's small fix +
ATA_HORKAGE_ATAPI_MOD16_DMA + pata_of_platform irq fix.

Gwendal posted patches to enable FIS based switching when NCQ is
disabled on sata_mv but I think this should be put into separate
branch and stay in linux-next till Mark Lord comes back and reviews it
as I don't know much about the driver and don't know of anyone other
than Gwendal and Mark who do.

sata_nv detection problem is still not resolved. I figured out
generic and ck804 but nf2/3 is still malfunctioning. I asked for more
tests and am waiting for results.

Eh... I guess that's about it. Enjoy your INBOX. :-P

--
tejun

2008-10-13 14:04:25

by Alan

[permalink] [raw]
Subject: Re: I have returned!

> I have returned! muhahahahahaha....

Oh good..
--

ibmtr: PCMCIA IBMTR is ok on 64bit

From: Alan Cox <[email protected]>

For whatever value of 'OK' can be applied to the use of token ring. Seems
the 32bit to 64bit cleanups missed re-enabling the pcmcia driver

Closes #7133 and also reviewed the code in question

Signed-off-by: Alan Cox <[email protected]>
---

drivers/net/pcmcia/Kconfig | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)


diff --git a/drivers/net/pcmcia/Kconfig b/drivers/net/pcmcia/Kconfig
index e8f55d8..9b8f793 100644
--- a/drivers/net/pcmcia/Kconfig
+++ b/drivers/net/pcmcia/Kconfig
@@ -111,7 +111,7 @@ config ARCNET_COM20020_CS

config PCMCIA_IBMTR
tristate "IBM PCMCIA tokenring adapter support"
- depends on IBMTR!=y && TR && !64BIT
+ depends on IBMTR!=y && TR
help
Say Y here if you intend to attach this type of Token Ring PCMCIA
card to your computer. You then also need to say Y to "Token Ring

2008-10-13 14:09:51

by Alan

[permalink] [raw]
Subject: [PATCH] fusion: remove dead mpt lan code

mptlan: Kill unused NAA workaround code

From: Alan Cox <[email protected]>

This triggers false bug reports as it does a bogus kmalloc with locks held
but is never really compiled

Closes #8329

Signed-off-by: Alan Cox <[email protected]>
---

drivers/message/fusion/mptlan.c | 108 ---------------------------------------
1 files changed, 0 insertions(+), 108 deletions(-)


diff --git a/drivers/message/fusion/mptlan.c b/drivers/message/fusion/mptlan.c
index a1abf95..603ffd0 100644
--- a/drivers/message/fusion/mptlan.c
+++ b/drivers/message/fusion/mptlan.c
@@ -77,12 +77,6 @@ MODULE_VERSION(my_VERSION);
* Fusion MPT LAN private structures
*/

-struct NAA_Hosed {
- u16 NAA;
- u8 ieee[FC_ALEN];
- struct NAA_Hosed *next;
-};
-
struct BufferControl {
struct sk_buff *skb;
dma_addr_t dma;
@@ -159,11 +153,6 @@ static u8 LanCtx = MPT_MAX_PROTOCOL_DRIVERS;
static u32 max_buckets_out = 127;
static u32 tx_max_out_p = 127 - 16;

-#ifdef QLOGIC_NAA_WORKAROUND
-static struct NAA_Hosed *mpt_bad_naa = NULL;
-DEFINE_RWLOCK(bad_naa_lock);
-#endif
-
/*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
/**
* lan_reply - Handle all data sent from the hardware.
@@ -780,30 +769,6 @@ mpt_lan_sdu_send (struct sk_buff *skb, struct net_device *dev)
// ctx, skb, skb->data));

mac = skb_mac_header(skb);
-#ifdef QLOGIC_NAA_WORKAROUND
-{
- struct NAA_Hosed *nh;
-
- /* Munge the NAA for Tx packets to QLogic boards, which don't follow
- RFC 2625. The longer I look at this, the more my opinion of Qlogic
- drops. */
- read_lock_irq(&bad_naa_lock);
- for (nh = mpt_bad_naa; nh != NULL; nh=nh->next) {
- if ((nh->ieee[0] == mac[0]) &&
- (nh->ieee[1] == mac[1]) &&
- (nh->ieee[2] == mac[2]) &&
- (nh->ieee[3] == mac[3]) &&
- (nh->ieee[4] == mac[4]) &&
- (nh->ieee[5] == mac[5])) {
- cur_naa = nh->NAA;
- dlprintk ((KERN_INFO "mptlan/sdu_send: using NAA value "
- "= %04x.\n", cur_naa));
- break;
- }
- }
- read_unlock_irq(&bad_naa_lock);
-}
-#endif

pTrans->TransactionDetails[0] = cpu_to_le32((cur_naa << 16) |
(mac[0] << 8) |
@@ -1572,79 +1537,6 @@ mpt_lan_type_trans(struct sk_buff *skb, struct net_device *dev)

fcllc = (struct fcllc *)skb->data;

-#ifdef QLOGIC_NAA_WORKAROUND
-{
- u16 source_naa = fch->stype, found = 0;
-
- /* Workaround for QLogic not following RFC 2625 in regards to the NAA
- value. */
-
- if ((source_naa & 0xF000) == 0)
- source_naa = swab16(source_naa);
-
- if (fcllc->ethertype == htons(ETH_P_ARP))
- dlprintk ((KERN_INFO "mptlan/type_trans: got arp req/rep w/ naa of "
- "%04x.\n", source_naa));
-
- if ((fcllc->ethertype == htons(ETH_P_ARP)) &&
- ((source_naa >> 12) != MPT_LAN_NAA_RFC2625)){
- struct NAA_Hosed *nh, *prevnh;
- int i;
-
- dlprintk ((KERN_INFO "mptlan/type_trans: ARP Req/Rep from "
- "system with non-RFC 2625 NAA value (%04x).\n",
- source_naa));
-
- write_lock_irq(&bad_naa_lock);
- for (prevnh = nh = mpt_bad_naa; nh != NULL;
- prevnh=nh, nh=nh->next) {
- if ((nh->ieee[0] == fch->saddr[0]) &&
- (nh->ieee[1] == fch->saddr[1]) &&
- (nh->ieee[2] == fch->saddr[2]) &&
- (nh->ieee[3] == fch->saddr[3]) &&
- (nh->ieee[4] == fch->saddr[4]) &&
- (nh->ieee[5] == fch->saddr[5])) {
- found = 1;
- dlprintk ((KERN_INFO "mptlan/type_trans: ARP Re"
- "q/Rep w/ bad NAA from system already"
- " in DB.\n"));
- break;
- }
- }
-
- if ((!found) && (nh == NULL)) {
-
- nh = kmalloc(sizeof(struct NAA_Hosed), GFP_KERNEL);
- dlprintk ((KERN_INFO "mptlan/type_trans: ARP Req/Rep w/"
- " bad NAA from system not yet in DB.\n"));
-
- if (nh != NULL) {
- nh->next = NULL;
- if (!mpt_bad_naa)
- mpt_bad_naa = nh;
- if (prevnh)
- prevnh->next = nh;
-
- nh->NAA = source_naa; /* Set the S_NAA value. */
- for (i = 0; i < FC_ALEN; i++)
- nh->ieee[i] = fch->saddr[i];
- dlprintk ((KERN_INFO "Got ARP from %02x:%02x:%02x:%02x:"
- "%02x:%02x with non-compliant S_NAA value.\n",
- fch->saddr[0], fch->saddr[1], fch->saddr[2],
- fch->saddr[3], fch->saddr[4],fch->saddr[5]));
- } else {
- printk (KERN_ERR "mptlan/type_trans: Unable to"
- " kmalloc a NAA_Hosed struct.\n");
- }
- } else if (!found) {
- printk (KERN_ERR "mptlan/type_trans: found not"
- " set, but nh isn't null. Evil "
- "funkiness abounds.\n");
- }
- write_unlock_irq(&bad_naa_lock);
- }
-}
-#endif

/* Strip the SNAP header from ARP packets since we don't
* pass them through to the 802.2/SNAP layers.

2008-10-13 14:14:37

by Alan

[permalink] [raw]
Subject: [PATCH] mptsas: remove pointless null check

mptsas: remove unneeded check

From: Alan Cox <[email protected]>

>From coverity checker. Closes #9675

Signed-off-by: Alan Cox <[email protected]>
---

drivers/message/fusion/mptsas.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)


diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 12b7325..a9019f0 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -2279,9 +2279,8 @@ mptsas_delete_expander_phys(MPT_ADAPTER *ioc)
mutex_lock(&ioc->sas_topology_mutex);
list_for_each_entry_safe(port_info, n, &ioc->sas_topology, list) {

- if (port_info->phy_info &&
- (!(port_info->phy_info[0].identify.device_info &
- MPI_SAS_DEVICE_INFO_SMP_TARGET)))
+ if (!(port_info->phy_info[0].identify.device_info &
+ MPI_SAS_DEVICE_INFO_SMP_TARGET))
continue;

if (mptsas_sas_expander_pg0(ioc, &buffer,

2008-10-13 14:23:22

by James Bottomley

[permalink] [raw]
Subject: Re: [PATCH] mptsas: remove pointless null check

On Mon, 2008-10-13 at 15:05 +0100, Alan Cox wrote:
> mptsas: remove unneeded check
>
> From: Alan Cox <[email protected]>
>
> >From coverity checker. Closes #9675
>
> Signed-off-by: Alan Cox <[email protected]>
> ---
>
> drivers/message/fusion/mptsas.c | 5 ++---
> 1 files changed, 2 insertions(+), 3 deletions(-)

-EWRONGLIST

This is a SCSI patch (although I admit with fusion sitting in
drivers/message it's hard to tell without looking in the MAINTAINERS
file).

>
> diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
> index 12b7325..a9019f0 100644
> --- a/drivers/message/fusion/mptsas.c
> +++ b/drivers/message/fusion/mptsas.c
> @@ -2279,9 +2279,8 @@ mptsas_delete_expander_phys(MPT_ADAPTER *ioc)
> mutex_lock(&ioc->sas_topology_mutex);
> list_for_each_entry_safe(port_info, n, &ioc->sas_topology, list) {
>
> - if (port_info->phy_info &&

If I remember rightly this check is necessary because phy_info can be
NULL in certain situations. Your patch will trip this to oops. What
your description needs to say is that we no longer need to check this
pointer for NULL because it was checked somewhere else in the stack ...
but I can't see where that is, where is it?

> - (!(port_info->phy_info[0].identify.device_info &
> - MPI_SAS_DEVICE_INFO_SMP_TARGET)))
> + if (!(port_info->phy_info[0].identify.device_info &
> + MPI_SAS_DEVICE_INFO_SMP_TARGET))
> continue;
>
> if (mptsas_sas_expander_pg0(ioc, &buffer,

James

2008-10-13 14:38:13

by Alan

[permalink] [raw]
Subject: Re: [PATCH] mptsas: remove pointless null check

> If I remember rightly this check is necessary because phy_info can be
> NULL in certain situations. Your patch will trip this to oops. What

In which case it will already have crashed

> your description needs to say is that we no longer need to check this
> pointer for NULL because it was checked somewhere else in the stack ...
> but I can't see where that is, where is it?

We don't check: but if it was NULL we then fall straight through and
dereference it unconditionally... so either its a wrong NULL check and
the old code oopses (which a trawl says it doesn't) or the check is bogus.

Alan

2008-10-13 23:33:20

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [PATCH] pata_of_platform: fix no irq handling

On Mon, 2008-10-13 at 15:56 +0900, Tejun Heo wrote:
> Anton Vorontsov wrote:
> > When no irq specified the pata_of_platform fills the irq_res with -1,
> > which is wrong to do for two reasons:
> >
> > 1. By definition, 'no irq' should be IRQ 0, not some negative integer;
> > 2. pata_platform checks for irq_res.start > 0, but since irq_res.start
> > is unsigned type, the check will be true for `-1'.
> >
> > Reported-by: Steven A. Falco <[email protected]>
> > Signed-off-by: Anton Vorontsov <[email protected]>
>
> applied to #tj-upstream (will soon be sent upstream)

Hrm... I applied it to powerpc.git too :-) Hopefully the merge will sort
it out !

Cheers,
Ben.

2008-10-16 09:40:56

by Jeff Garzik

[permalink] [raw]
Subject: Re: I have returned!

Alan Cox wrote:
>> I have returned! muhahahahahaha....
>
> Oh good..
> --
>
> ibmtr: PCMCIA IBMTR is ok on 64bit
>
> From: Alan Cox <[email protected]>
>
> For whatever value of 'OK' can be applied to the use of token ring. Seems
> the 32bit to 64bit cleanups missed re-enabling the pcmcia driver
>
> Closes #7133 and also reviewed the code in question
>
> Signed-off-by: Alan Cox <[email protected]>
> ---
>
> drivers/net/pcmcia/Kconfig | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)

applied