linux-kernel - Re: [PATCH] drivers:net:hamradio: Removing Depricated IRQF

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <52068B3C.4030707@bfs.de>
Date:	Sat, 10 Aug 2013 20:49:32 +0200
From:	walter harms <wharms@....de>
To:	Kumar Gaurav <kumargauravgupta3@...il.com>
CC:	Julia Lawall <julia.lawall@...6.fr>, t.sailer@...mni.ethz.ch,
	jreuter@...na.de, jpr@...bb.org, linux-hams@...r.kernel.org,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] drivers:net:hamradio: Removing Depricated IRQF_DISABLED

Hello Kumar,
i will try to resolve the riddle ...

doing something like fn(a,b,c,0) is a bad habit.
0 is called a "magic number" because noone reading the code has an idea
why it is there. Using the name IRQF_TRIGGER_NONE good.

Having a named bit like IRQ_TRIGGER|IRQ_NMI is most times also a good idea
because it correspond (most times) to the naming of the bits in the data sheet.

The problems starts when this hits reality since nothing is clearly black or white.
In this case when you remove IRQF_DISABLED disables "bit" something should be left.

a|b|IRQF_DISABLED -> a|b  (that is the easy part)
but
IRQF_DISABLED -> IRQF_TRIGGER_NONE

Otherwise you would be left with "0". That is a magic number (see above).
So this is to be replaced by a placeholder called IRQF_TRIGGER_NONE.

hope that really helps and that i have understood the discussion.

re,
 wh


Am 10.08.2013 20:15, schrieb Kumar Gaurav:
> On Saturday 10 August 2013 10:54 PM, Julia Lawall wrote:
>> On Sat, 10 Aug 2013, Kumar Gaurav wrote:
>>
>>> On Saturday 10 August 2013 10:17 PM, Julia Lawall wrote:
>>>> If it is in a | with something else, I think you can just remove it.
>>>>
>>>> julia
>>>>
>>>> On Sat, 10 Aug 2013, Kumar Gaurav wrote:
>>>>
>>>>> Removed IRQF_DISABLED as it's deprecated and should be removed
>>>>>
>>>>> Signed-off-by: Kumar Gaurav <kumargauravgupta3@...il.com>
>>>>> ---
>>>>>    drivers/net/hamradio/baycom_ser_fdx.c |    2 +-
>>>>>    drivers/net/hamradio/baycom_ser_hdx.c |    2 +-
>>>>>    drivers/net/hamradio/scc.c            |    2 +-
>>>>>    drivers/net/hamradio/yam.c            |    2 +-
>>>>>    4 files changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/hamradio/baycom_ser_fdx.c
>>>>> b/drivers/net/hamradio/baycom_ser_fdx.c
>>>>> index a974727..c114009 100644
>>>>> --- a/drivers/net/hamradio/baycom_ser_fdx.c
>>>>> +++ b/drivers/net/hamradio/baycom_ser_fdx.c
>>>>> @@ -445,7 +445,7 @@ static int ser12_open(struct net_device *dev)
>>>>>        outb(0, FCR(dev->base_addr));  /* disable FIFOs */
>>>>>        outb(0x0d, MCR(dev->base_addr));
>>>>>        outb(0, IER(dev->base_addr));
>>>>> -    if (request_irq(dev->irq, ser12_interrupt, IRQF_DISABLED |
>>>>> IRQF_SHARED,
>>>>> +    if (request_irq(dev->irq, ser12_interrupt, IRQF_TRIGGER_NONE |
>>>>> IRQF_SHARED,
>>>>>                "baycom_ser_fdx", dev)) {
>>>>>            release_region(dev->base_addr, SER12_EXTENT);
>>>>>            return -EBUSY;
>>>>> diff --git a/drivers/net/hamradio/baycom_ser_hdx.c
>>>>> b/drivers/net/hamradio/baycom_ser_hdx.c
>>>>> index e349d86..d91c1fd 100644
>>>>> --- a/drivers/net/hamradio/baycom_ser_hdx.c
>>>>> +++ b/drivers/net/hamradio/baycom_ser_hdx.c
>>>>> @@ -490,7 +490,7 @@ static int ser12_open(struct net_device *dev)
>>>>>        outb(0, FCR(dev->base_addr));  /* disable FIFOs */
>>>>>        outb(0x0d, MCR(dev->base_addr));
>>>>>        outb(0, IER(dev->base_addr));
>>>>> -    if (request_irq(dev->irq, ser12_interrupt, IRQF_DISABLED |
>>>>> IRQF_SHARED,
>>>>> +    if (request_irq(dev->irq, ser12_interrupt, IRQF_TRIGGER_NONE |
>>>>> IRQF_SHARED,
>>>>>                "baycom_ser12", dev)) {
>>>>>            release_region(dev->base_addr, SER12_EXTENT);
>>>>>            return -EBUSY;
>>>>> diff --git a/drivers/net/hamradio/scc.c b/drivers/net/hamradio/scc.c
>>>>> index bc1d521..4bc6ee8 100644
>>>>> --- a/drivers/net/hamradio/scc.c
>>>>> +++ b/drivers/net/hamradio/scc.c
>>>>> @@ -1734,7 +1734,7 @@ static int scc_net_ioctl(struct net_device *dev,
>>>>> struct ifreq *ifr, int cmd)
>>>>>                if (!Ivec[hwcfg.irq].used && hwcfg.irq)
>>>>>                {
>>>>>                    if (request_irq(hwcfg.irq, scc_isr,
>>>>> -                        IRQF_DISABLED, "AX.25 SCC",
>>>>> +                        0, "AX.25 SCC",
>>>>>                            (void *)(long) hwcfg.irq))
>>>>>                        printk(KERN_WARNING "z8530drv:
>>>>> warning, cannot get IRQ %d\n", hwcfg.irq);
>>>>>                    else
>>>>> diff --git a/drivers/net/hamradio/yam.c b/drivers/net/hamradio/yam.c
>>>>> index 0721e72..f947887 100644
>>>>> --- a/drivers/net/hamradio/yam.c
>>>>> +++ b/drivers/net/hamradio/yam.c
>>>>> @@ -888,7 +888,7 @@ static int yam_open(struct net_device *dev)
>>>>>            goto out_release_base;
>>>>>        }
>>>>>        outb(0, IER(dev->base_addr));
>>>>> -    if (request_irq(dev->irq, yam_interrupt, IRQF_DISABLED |
>>>>> IRQF_SHARED,
>>>>> dev->name, dev)) {
>>>>> +    if (request_irq(dev->irq, yam_interrupt, IRQF_TRIGGER_NONE |
>>>>> IRQF_SHARED, dev->name, dev)) {
>>>>>            printk(KERN_ERR "%s: irq %d busy\n", dev->name, dev->irq);
>>>>>            ret = -EBUSY;
>>>>>            goto out_release_base;
>>>>> -- 
>>>>> 1.7.9.5
>>>>>
>>>>> -- 
>>>>> To unsubscribe from this list: send the line "unsubscribe
>>>>> kernel-janitors"
>>>>> in
>>>>> the body of a message to majordomo@...r.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>
>>> Yes i was suggested for using IRQF_TRIGGER_NONE too so used this.
>>> Please tell
>>> me if this makes any difference or issue?
>> I think that the suggestion was to use IRQF_TRIGGER_NONE only when
>> removing IRQF_DISABLED would leave nothing left.
>>
>> - IRQF_DISABLED | e
>> + e
>>
>> otherwise,
>>
>> - IRQF_DISABLED
>> + 0
>>
>> julia
> Sorry to prompt back but i'm confused. if removing IRQF_DISABLED leaves
> nothing then should i use 0 or IRQF_TRIGGER_NONE?
> I'm convinced with
> 
>     - IRQF_DISABLED | e
>     + e
> 
> Regards
> 
> Kumar Gaurav
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-hams" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/