linux-kernel - Re: [PATCH] scsi: mac_scsi: Replace NO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <009bf4b2-ab5d-b583-20bd-8eee397e251a@csgroup.eu>
Date:   Thu, 6 Oct 2022 11:13:52 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Finn Thain <fthain@...ux-m68k.org>
CC:     Michael Schmitz <schmitzmic@...il.com>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>
Subject: Re: [PATCH] scsi: mac_scsi: Replace NO_IRQ by 0

Le 06/10/2022 à 11:25, Finn Thain a écrit :
> On Thu, 6 Oct 2022, Christophe Leroy wrote:
> 
>> NO_IRQ is used to check the return of irq_of_parse_and_map().
>>
>> On some architecture NO_IRQ is 0, on other architectures it is -1.
>>
> 
> Yes. The core NCR5380 driver is used on ARM etc. where NO_IRQ is -1 as
> well as on powerpc where it is 0.
> 
>> irq_of_parse_and_map() returns 0 on error, independent of NO_IRQ.
>>
>> So use 0 instead of using NO_IRQ.
>>
> 
> Sorry, I must be missing something.

My mistake.

I started by removing NO_IRQ definition in powerpc and then fixed all 
build failures by replacing NO_IRQ by 0. Then I splitted the patch into 
one per subsystem, all with the same explaination.

Most places it was just a verification of the value returned by 
irq_of_parse_and_map() where it is obviously wrong to use NO_IRQ, 
especially on ARM where NO_IRQ doesn't match what irq_of_parse_and_map() 
returns in case on error.

But here in the mac_scsi driver it seems a bit different and I have a 
look more closely.

> 
> You seem to be saying that this driver could be re-used in the context of
> openfirmware/device trees if it avoided using the NO_IRQ. Do I have that
> right?
> 
> Or are you changing NO_IRQ semantics tree-wide for some reason explained
> somewhere else?

No, I only say that NO_IRQ doesn't match the value returned by 
irq_of_parse_and_map(). Ultimately I want to remove the #define NO_IRQ 
from arch/powerpc/include/asm/irq.h

That's to be linked to following message from Linus : 
https://lkml.org/lkml/2005/11/21/221

> 
> If it is the former, shouldn't you reverse the comment in
> arch/powerpc/include/asm/irq.h, which says the macro is to be used in the
> way this driver (and others) use it?
> 
> If it is the latter, shouldn't you address the use of NO_IRQ in the core
> NCR5380 driver rather than just this wrapper?

Yes I guess so.

> 
> Moreover, wouldn't it make more sense to fix the callers of
> irq_of_parse_and_map(), since they appear to be abusing the NO_IRQ macro?

Indeed. That's what is being done most places.

> 
> For example, drivers/ata/sata_dwc_460ex.c actually does #define NO_IRQ 0
> and then expects irq_of_parse_and_map() will somehow use the same value to
> mean the same thing...

It didn't pop up during the multi-build I did for powerpc, so I guess 
that driver is not used for powerpc ? In the ata subsystem I fixed 
pata_mpc52xx.

Thanks
Christophe