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 11:39:49 +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 10:39, 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.
> 
> 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