linux-kernel - Re: [RFC PATCH v2 05/15] usb:cdns3: Added DRD support

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, 26 Nov 2018 12:15:20 +0200
From:   Roger Quadros <rogerq@...com>
To:     Pawel Laszczak <pawell@...ence.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
CC:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "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: [RFC PATCH v2 05/15] usb:cdns3: Added DRD support

On 26/11/18 12:09, Pawel Laszczak wrote:
>>>>
>>>> Pawel,
>>>>
>>>> On 26/11/18 09:23, Pawel Laszczak wrote:
>>>>> Hi Roger,
>>>>>
>>>>>> On 18/11/18 12:09, Pawel Laszczak wrote:
>>>>>>> Patch adds supports for detecting Host/Device mode.
>>>>>>> Controller has additional OTG register that allow
>>>>>>> implement even whole OTG functionality.
>>>>>>> At this moment patch adds support only for detecting
>>>>>>> the appropriate mode based on strap pins and ID pin.
>>>>>>>
>>>>>>> Signed-off-by: Pawel Laszczak <pawell@...ence.com>
>>>>>>> ---
>>>>>>>  drivers/usb/cdns3/Makefile |   2 +-
>>>>>>>  drivers/usb/cdns3/core.c   |  27 +++--
>>>>>>>  drivers/usb/cdns3/drd.c    | 229 +++++++++++++++++++++++++++++++++++++
>>>>>>>  drivers/usb/cdns3/drd.h    | 122 ++++++++++++++++++++
>>>>>>>  4 files changed, 372 insertions(+), 8 deletions(-)
>>>>>>>  create mode 100644 drivers/usb/cdns3/drd.c
>>>>>>>  create mode 100644 drivers/usb/cdns3/drd.h
>>>>>>>
>>>>>>> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
>>>>>>> index 02d25b23c5d3..e779b2a2f8eb 100644
>>>>>>> --- a/drivers/usb/cdns3/Makefile
>>>>>>> +++ b/drivers/usb/cdns3/Makefile
>>>>>>> @@ -1,5 +1,5 @@
>>>>>>>  obj-$(CONFIG_USB_CDNS3)			+= cdns3.o
>>>>>>>  obj-$(CONFIG_USB_CDNS3_PCI_WRAP)	+= cdns3-pci.o
>>>>>>>
>>>>>>> -cdns3-y					:= core.o
>>>>>>> +cdns3-y					:= core.o drd.o
>>>>>>>  cdns3-pci-y		 		:= cdns3-pci-wrap.o
>>>>>>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>>>>>>> index f9055d4da67f..dbee4325da7f 100644
>>>>>>> --- a/drivers/usb/cdns3/core.c
>>>>>>> +++ b/drivers/usb/cdns3/core.c
>>>>>>> @@ -17,6 +17,7 @@
>>>>>>>
>>>>>>>  #include "gadget.h"
>>>>>>>  #include "core.h"
>>>>>>> +#include "drd.h"
>>>>>>>
>>>>>>>  static inline struct cdns3_role_driver *cdns3_get_current_role_driver(struct cdns3 *cdns)
>>>>>>>  {
>>>>>>> @@ -57,8 +58,10 @@ static inline void cdns3_role_stop(struct cdns3 *cdns)
>>>>>>>  static enum cdns3_roles cdns3_get_role(struct cdns3 *cdns)
>>>>>>>  {
>>>>>>>  	if (cdns->roles[CDNS3_ROLE_HOST] && cdns->roles[CDNS3_ROLE_GADGET]) {
>>>>>>> -		//TODO: implements selecting device/host mode
>>>>>>> -		return CDNS3_ROLE_HOST;
>>>>>>> +		if (cdns3_is_host(cdns))
>>>>>>> +			return CDNS3_ROLE_HOST;
>>>>>>> +		if (cdns3_is_device(cdns))
>>>>>>> +			return CDNS3_ROLE_GADGET;
>>>>>>>  	}
>>>>>>>  	return cdns->roles[CDNS3_ROLE_HOST]
>>>>>>>  		? CDNS3_ROLE_HOST
>>>>>>> @@ -124,6 +127,12 @@ static irqreturn_t cdns3_irq(int irq, void *data)
>>>>>>>  	struct cdns3 *cdns = data;
>>>>>>>  	irqreturn_t ret = IRQ_NONE;
>>>>>>>
>>>>>>> +	if (cdns->dr_mode == USB_DR_MODE_OTG) {
>>>>>>> +		ret = cdns3_drd_irq(cdns);
>>>>>>> +		if (ret == IRQ_HANDLED)
>>>>>>> +			return ret;
>>>>>>> +	}
>>>>>>
>>>>>> The kernel's shared IRQ model takes care of sharing the same interrupt
>>>>>> between different devices and their drivers. You don't need to manually
>>>>>> handle it here. Just let all 3 drivers do a request_irq() and have
>>>>>> handlers check if the IRQ was theirs or not and return IRQ_HANDLED or
>>>>>> IRQ_NONE accordingly.
>>>>>>
>>>>>> Looks like you can do away with irq member of the role driver struct.
>>>>>
>>>>> Ok, I will split it into 3 separate part, but in this case, I additionally have to check the current
>>>>> role in ISR function. Driver can't read host side registers when controller works in device role
>>>>> and vice versa. One part of controller is kept in reset. Only DRD registers are common and are all accessible.
>>>>>
>>>>
>>>> In which ISR do you need to check current role?
>>>>
>>>> I'm not sure if we are on the same page.
>>>> Core (drd) driver shouldn't read host/device side registers. All 3 drivers,
>>>> i.e. DRD(core), Host (xhci) and device (cdns3) should do a request_irq()
>>>> and process their respective IRQ events.
>>>
>>> Yes, I understand.
>>> I need to check this in cdns3_irq_handler_thread and cdns3_host_irq.
>>>
>>> Core (drd) has register that are always accessible.
>>> Core (device)  - registers are available also in device mode
>>> Core (host)  - registers  are available only in host mode
>>>
>>> So we can use separate request_irq  for drd, device and host side, but
>>> we need ensure that in host side driver will not touch the device register.
>>
>> That should never happen as host side doesn't have visibility to device registers.
> 
> I meant the following scenario:
> Assume that controller work in Device role and it raise interrupt for Device part. 
> Now host is kept in reset. 
> 1. System receive interrupt 
> 2.  Kernel call drd_irq but this function return IRQ_NONE
> 3.  Kernel call cdns3_host_irq and driver has to read some interrupt register to check if 
>      this event is addressed to him. In this moment we can have unknown behavior. 

This is the problem. If device is not able to figure out its interrupt event then
it must unregister the ISR.

> 4.  Kernel call cdns3_irq_handler  (device). This point probably will not happen because 
>     It could have stuck at point 3. 
> 
> Ok, I think I understood. After changing role, the driver unregisters the interrupt function
> used by previous role so point 3 never happen. 
> 
> I will try to check how it works.  
> 

Cool! Thanks.

>>>
>>> We doesn't know the order in which  the system will call interrupts function related to
>>>  the shared interrupt line.
>>
>> Order shouldn't matter. Each user will check its own events return IRQ_NONE if they
>> didn't cause the IRQ.
>>>
>>
>> <snip>
>>

cheers,
-roger
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki