linux-kernel - Re: [PATCHv6 1/3] usb: USB Type-C connector class

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: <fbf390ec-4912-1fb5-f11b-0afc26273924@roeck-us.net>
Date:   Tue, 30 Aug 2016 06:46:24 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Oliver Neukum <oneukum@...e.com>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        Vincent Palatin <vpalatin@...omium.org>,
        Bin Gao <bin.gao@...ux.intel.com>,
        Felipe Balbi <felipe.balbi@...ux.intel.com>,
        linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCHv6 1/3] usb: USB Type-C connector class

On 08/30/2016 03:04 AM, Heikki Krogerus wrote:
> Hi Oliver,
>
> On Tue, Aug 30, 2016 at 11:32:01AM +0200, Oliver Neukum wrote:
>> On Mon, 2016-08-29 at 15:36 +0300, Heikki Krogerus wrote:
>>> +What:          /sys/class/typec/<port>/current_data_role
>>> +Date:          June 2016
>>> +Contact:       Heikki Krogerus <heikki.krogerus@...ux.intel.com>
>>> +Description:
>>> +               The current USB data role the port is operating in.
>>> This
>>> +               attribute can be used for requesting data role
>>> swapping on the
>>> +               port. Swapping is only supported as an asynchronous
>>> operation
>>> +               and requires polling of the attribute in order to know
>>> the
>>> +               result, so successful write operation does not mean
>>> successful
>>> +               swap.
>>> +
>>
>> That is badly formulated. Does it mean that poll() or select()
>> can be used or does the value need to be repearedly read?
>
> Does polling not always mean poll/select?
>
>> And how would you learn about an error?
>
> This is what I'm also really worried about. I'm now wondering did I
> give up too easily on this to Guenter in hope to move this thing
> forward. He said it's problematic to do these calls synchronously for
> him. Was it something related to potential conflicting role swaps from
> both ends?
>
> Guenter, can you please elaborate? And how do you plan to report
> failures with the swaps?
>

I thought we had this sorted out. When I said "asynchronous", I did not mean
that the sysfs operation would not wait for the operation to complete. I meant
that the Type-C state machine operates in a different context than the sysfs/class
code. Since the state machine operates in a different context, it may have
to execute a callback into the class code at any time, independently of
any pending role changes triggered through sysfs. Please have a look into
the patch set I submitted for details. Roughly it works as follows.

Class code context				State machine context

User requests role change
Class code calls {dr,pr,vconn}_set
{dr,pr,vconn}_set code validates request
{dr,pr,vconn}_set code sends role change
	request to state machine		State machine gets role change request
{dr,pr,vconn}_set code waits for completion
						State machine sends role change request
						to link partner
						Partner reports Accept or Reject
						State machine changes state as requested
						State machine reports new role to class code
							via callbacks
						State machine informs Class code that request
						is complete
{dr,pr,vconn}_set code gets results
	and returns to caller
Class code reports results to user

 From user perspective, everything is synchronous. However, the state machine has to be
able to run independently and report role and other state changes to the class code while
a role change request from the class code is pending. For example, it has to be able to
handle incoming role change requests from the link partner, and it has to be able to
handle link state changes. All those have to be reported to the class code. This is
impossible if the class code holds a lock while a role change triggered from user space
is pending, which is why I asked for the locks in the class code to be removed.

Maybe my use of the term "asynchronous" was misleading, and I should have said "operates
in a different context" instead. My apologies.

Thanks,
Guenter