linux-kernel - RE: [PATCH v1 2/2] usb:cdns3 Add Cadence USB3 DRD Driver

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]

Date:   Mon, 17 Dec 2018 11:51:50 +0000
From:   Pawel Laszczak <pawell@...ence.com>
To:     Felipe Balbi <balbi@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Kishon Vijay Abraham I <kishon@...com>
CC:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "rogerq@...com" <rogerq@...com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Alan Douglas <adouglas@...ence.com>,
        "jbergsagel@...com" <jbergsagel@...com>,
        "nsekhar@...com" <nsekhar@...com>, "nm@...com" <nm@...com>,
        Suresh Punnoose <sureshp@...ence.com>,
        "peter.chen@....com" <peter.chen@....com>,
        Pawel Jez <pjez@...ence.com>, Rahul Kumar <kurahul@...ence.com>
Subject: RE: [PATCH v1 2/2] usb:cdns3 Add Cadence USB3 DRD Driver

Hi,

>
>>>> +static irqreturn_t cdns3_device_irq_handler(int irq, void *data)
>>>> +{
>>>> +	struct cdns3_device *priv_dev;
>>>> +	struct cdns3 *cdns = data;
>>>> +	irqreturn_t ret = IRQ_NONE;
>>>> +	unsigned long flags;
>>>> +	u32 reg;
>>>> +
>>>> +	priv_dev = cdns->gadget_dev;
>>>> +	spin_lock_irqsave(&priv_dev->lock, flags);
>>>
>>>you're already running in hardirq context. Why do you need this lock at
>>>all? I would be better to use the hardirq handler to mask your
>>>interrupts, so they don't fire again, then used the top-half (softirq)
>>>handler to actually handle the interrupts.
>>
>> Yes, spin_lock_irqsave is not necessary here.
>>
>> Do you mean replacing devm_request_irq with a request_threaded_irq ?
>> I have single interrupt line shared between  Host, Driver, DRD/OTG.
>> I'm not sure if it will work more efficiently.
>
>The whole idea for running very little in hardirq context is to give the
>scheduler a chance to decide what should run. This is important to
>reduce latency when running with RT patchset applied, for
>example. However, I'll give you that, it's a minor requirement. It's
>just that, to me, it's a small detail that's easy to implement.

I will do it in PATCH v2 or PATCH v3.  
I need to post next version before 24 of December, so if I can do it
before this date then it will be in PATCH v2. 

Thanks 
Pawel