linux-kernel - Re: [PATCHv2 1/2] extcon: add driver for Intel USB mux

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, 07 Dec 2015 10:24:22 +0900
From:	Chanwoo Choi <cw00.choi@...sung.com>
To:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	MyungJoo Ham <myungjoo.ham@...sung.com>,
	David Cohen <david.a.cohen@...ux.intel.com>,
	Lu Baolu <baolu.lu@...ux.intel.com>,
	Mathias Nyman <mathias.nyman@...ux.intel.com>,
	Felipe Balbi <balbi@...com>, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCHv2 1/2] extcon: add driver for Intel USB mux

Hi,

On 2015년 12월 04일 17:51, Heikki Krogerus wrote:
> Hi,
> 
>> I do never want to add some specific funtcion for only this driver.
>> I think is not appropriate way.
>> - intel_usb_mux_unregister
>> - intel_usb_mux_register
>>
>> The client driver using extcon driver should use the standard extcon API
>> for code consistency. Also, I'll do the more detailed review for this patch.
> 
> The internal mux we are controlling here is physically separate
> device. Ideally we could populate child device for it, but since that
> is not possible because of the resource conflict, we use the library
> approach, which is really not that uncommon.

New added functions for only specific device driver is not library. 

The all device drivers which is included in some framework should
connect to the other device driver through only framework API as following:
	--------------------          ----------------
	| EXTCON framework |<-------->| USB framework |
	--------------------          -----------------	
		|                            |
		|                            |
	extcon-intel-usb.c             pci-quirks.c

But, in this case, added funticon is just direct call function
without any standard API. The below case is never appropriate implementation.

	--------------------          ----------------
	| EXTCON framework |          | USB framework |
	--------------------          -----------------	
		|                            |
		|                            |
	extcon-intel-usb.c <--------  pci-quirks.c

> 
> I don't think I agree with your point even at general level. The
> control required to handle this mux, even though simple, is enough to
> deserve to be separated from xHCI code. xHCI should not need to care
> about anything else expect does it have the mux, i.e. does it need to
> register it or not. It should not need to care about how it needs to
> be controlled or even what it is. We may decide to create something
> else out of it instead of an extcon device later.
> 
> But in any case, the mux is available on all new Intel platforms, but
> it needs to be controlled by OS only in few "special" cases. We can
> not force xHCI (or pci-quirks.c to be more precise) to be aware of
> these "special" cases. The only way to make it work like that would
> bet by using ifdefs, and we really really don't want that.
> 
> And please also note that, though for now we only expect the mux
> control registers to be part of xHCI MMIO, that is not always the
> case. The control registers are part of the device controller MMIO on
> some platforms. We do not want to duplicate the whole control of the
> mux if/when we need the OS to be in control of it on a platform that
> has those control registers mapped somewhere else then xHCI MMIO,
> 
> So I would say that we have pretty good justification for separating
> the mux control, which means unfortunately custom API in this case.
> 
> But if you would prefer that we put the files somewhere else then
> drivers/extcon/ and include/linux/extcon/ I'm fine with that. If you
> like, we can put it to drivers/usb/host/ as that is where
> pci-quirks.c is. That way I think we can also put the header to
> include/usb/.

There are the two type of extcon drivers as following:
- provider extcon driver which use the devm_extcon_dev_register() and extcon_set_cable_state().
- client extcon driver which use the extcon_register_notifier() and extcon_set_cable_state() usually.
The drivers/extcon directory only includes the provider extcon driver.

You make the extcon-intel-usb.c as provider extcon driver.
At the same time, this driver is used for client extcon driver
by using the extcon_register_notifier(). If you want to recevie
the notification from extcon_register_notifier(), the extcon device
should update the state of external connector throught extcon_set_cable_state().
But, this driver don' use the extcon_set_cable_state().

I think that you should divide it according to role.

Again, the usage case of extcon have to consist of both provider extcon driver
and client extcon driver. If there is no provider extcon driver,
client extcon driver don't receive the any notification of external connector
from provider extcon driver.

Thanks,
Chanwoo Choi




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/