| 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
| ||
|
Message-ID: <BYAPR07MB4709DFA347BC883F7F276332DDD70@BYAPR07MB4709.namprd07.prod.outlook.com>
Date: Mon, 26 Nov 2018 10:09:48 +0000
From: Pawel Laszczak <pawell@...ence.com>
To: Roger Quadros <rogerq@...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
>>>
>>> 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.
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.
>>
>> 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
Thanks,
Cheers,
Pawel
Powered by blists - more mailing lists