[PATCH] scsi: mac_scsi: Replace NO_IRQ by 0

[PATCH] scsi: mac_scsi: Replace NO_IRQ by 0

[Christophe Leroy]

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.

irq_of_parse_and_map() returns 0 on error, independent of NO_IRQ.

So use 0 instead of using NO_IRQ.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 drivers/scsi/mac_scsi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/mac_scsi.c b/drivers/scsi/mac_scsi.c
index 2e511697fce3..e4df2b501e8b 100644
--- a/drivers/scsi/mac_scsi.c
+++ b/drivers/scsi/mac_scsi.c
@@ -478,7 +478,7 @@ static int __init mac_scsi_probe(struct platform_device *pdev)
 	if (irq)
 		instance->irq = irq->start;
 	else
-		instance->irq = NO_IRQ;
+		instance->irq = 0;
 
 	hostdata = shost_priv(instance);
 	hostdata->base = pio_mem->start;
@@ -495,7 +495,7 @@ static int __init mac_scsi_probe(struct platform_device *pdev)
 	if (error)
 		goto fail_init;
 
-	if (instance->irq != NO_IRQ) {
+	if (instance->irq) {
 		error = request_irq(instance->irq, macscsi_intr, IRQF_SHARED,
 		                    "NCR5380", instance);
 		if (error)
@@ -514,7 +514,7 @@ static int __init mac_scsi_probe(struct platform_device *pdev)
 	return 0;
 
 fail_host:
-	if (instance->irq != NO_IRQ)
+	if (instance->irq)
 		free_irq(instance->irq, instance);
 fail_irq:
 	NCR5380_exit(instance);
@@ -528,7 +528,7 @@ static int __exit mac_scsi_remove(struct platform_device *pdev)
 	struct Scsi_Host *instance = platform_get_drvdata(pdev);
 
 	scsi_remove_host(instance);
-	if (instance->irq != NO_IRQ)
+	if (instance->irq)
 		free_irq(instance->irq, instance);
 	NCR5380_exit(instance);
 	scsi_host_put(instance);
-- 
2.37.1

Re: [PATCH] scsi: mac_scsi: Replace NO_IRQ by 0

[Finn Thain]

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.

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?

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?

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?

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...

> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  drivers/scsi/mac_scsi.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/mac_scsi.c b/drivers/scsi/mac_scsi.c
> index 2e511697fce3..e4df2b501e8b 100644
> --- a/drivers/scsi/mac_scsi.c
> +++ b/drivers/scsi/mac_scsi.c
> @@ -478,7 +478,7 @@ static int __init mac_scsi_probe(struct platform_device *pdev)
>  	if (irq)
>  		instance->irq = irq->start;
>  	else
> -		instance->irq = NO_IRQ;
> +		instance->irq = 0;
>  
>  	hostdata = shost_priv(instance);
>  	hostdata->base = pio_mem->start;
> @@ -495,7 +495,7 @@ static int __init mac_scsi_probe(struct platform_device *pdev)
>  	if (error)
>  		goto fail_init;
>  
> -	if (instance->irq != NO_IRQ) {
> +	if (instance->irq) {
>  		error = request_irq(instance->irq, macscsi_intr, IRQF_SHARED,
>  		                    "NCR5380", instance);
>  		if (error)
> @@ -514,7 +514,7 @@ static int __init mac_scsi_probe(struct platform_device *pdev)
>  	return 0;
>  
>  fail_host:
> -	if (instance->irq != NO_IRQ)
> +	if (instance->irq)
>  		free_irq(instance->irq, instance);
>  fail_irq:
>  	NCR5380_exit(instance);
> @@ -528,7 +528,7 @@ static int __exit mac_scsi_remove(struct platform_device *pdev)
>  	struct Scsi_Host *instance = platform_get_drvdata(pdev);
>  
>  	scsi_remove_host(instance);
> -	if (instance->irq != NO_IRQ)
> +	if (instance->irq)
>  		free_irq(instance->irq, instance);
>  	NCR5380_exit(instance);
>  	scsi_host_put(instance);
> 

Re: [PATCH] scsi: mac_scsi: Replace NO_IRQ by 0

[Christophe Leroy]

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