| 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
| ||
|
Date: Mon, 14 Mar 2016 09:09:22 +0800
From: Lu Baolu <baolu.lu@...ux.intel.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Felipe Balbi <balbi@...nel.org>,
Mathias Nyman <mathias.nyman@...el.com>,
Lee Jones <lee.jones@...aro.org>,
Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
MyungJoo Ham <myungjoo.ham@...sung.com>,
Chanwoo Choi <cw00.choi@...sung.com>,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port
mux
Hi,
On 03/11/2016 08:06 AM, Greg Kroah-Hartman wrote:
> On Tue, Mar 08, 2016 at 03:53:44PM +0800, Lu Baolu wrote:
>> Several Intel PCHs and SOCs have an internal mux that is used to
>> share one USB port between device controller and host controller.
>>
>> A usb port mux could be abstracted as the following elements:
>> 1) mux state: HOST or PERIPHERAL;
>> 2) an extcon cable which triggers the change of mux state between
>> HOST and PERIPHERAL;
>> 3) The required action to do the real port switch.
>>
>> This patch adds the common code to handle usb port mux. With this
>> common code, the individual mux driver, which always is platform
>> dependent, could focus on the real operation of mux switch.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com>
>> Reviewed-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
>> Reviewed-by: Felipe Balbi <balbi@...nel.org>
>> ---
>> Documentation/ABI/testing/sysfs-bus-platform | 15 +++
>> MAINTAINERS | 7 ++
>> drivers/usb/Kconfig | 2 +
>> drivers/usb/Makefile | 1 +
>> drivers/usb/mux/Kconfig | 9 ++
>> drivers/usb/mux/Makefile | 4 +
>> drivers/usb/mux/intel-mux.c | 166 +++++++++++++++++++++++++++
>> include/linux/usb/intel-mux.h | 47 ++++++++
>> 8 files changed, 251 insertions(+)
>> create mode 100644 drivers/usb/mux/Kconfig
>> create mode 100644 drivers/usb/mux/Makefile
>> create mode 100644 drivers/usb/mux/intel-mux.c
>> create mode 100644 include/linux/usb/intel-mux.h
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-platform b/Documentation/ABI/testing/sysfs-bus-platform
>> index 5172a61..a2261cb 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-platform
>> +++ b/Documentation/ABI/testing/sysfs-bus-platform
>> @@ -18,3 +18,18 @@ Description:
>> devices to opt-out of driver binding using a driver_override
>> name such as "none". Only a single driver may be specified in
>> the override, there is no support for parsing delimiters.
>> +
>> +What: /sys/bus/platform/devices/.../intel_mux
>> +Date: Febuary 2016
>> +Contact: Lu Baolu <baolu.lu@...ux.intel.com>
>> +Description:
>> + In some platforms, a single USB port is shared between a USB host
>> + controller and a device controller. A USB mux driver is needed to
>> + handle the port mux. intel_mux attribute shows and stores the mux
>> + state.
>> + For read:
>> + 'peripheral' - mux switched to PERIPHERAL controller;
>> + 'host' - mux switched to HOST controller.
>> + For write:
>> + 'peripheral' - mux will be switched to PERIPHERAL controller;
>> + 'host' - mux will be switched to HOST controller.
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index c55b37e..a93d26a 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -11400,6 +11400,13 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git
>> S: Maintained
>> F: drivers/usb/phy/
>>
>> +USB PORT MUX DRIVER
>> +M: Lu Baolu <baolu.lu@...ux.intel.com>
>> +L: linux-usb@...r.kernel.org
>> +S: Supported
>> +F: drivers/usb/mux/intel-mux.c
>> +F: include/linux/usb/intel-mux.h
>> +
>> USB PRINTER DRIVER (usblp)
>> M: Pete Zaitcev <zaitcev@...hat.com>
>> L: linux-usb@...r.kernel.org
>> diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
>> index 8ed451d..dbd6620 100644
>> --- a/drivers/usb/Kconfig
>> +++ b/drivers/usb/Kconfig
>> @@ -149,6 +149,8 @@ endif # USB
>>
>> source "drivers/usb/phy/Kconfig"
>>
>> +source "drivers/usb/mux/Kconfig"
>> +
>> source "drivers/usb/gadget/Kconfig"
>>
>> config USB_LED_TRIG
>> diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
>> index dca7856..9a92338 100644
>> --- a/drivers/usb/Makefile
>> +++ b/drivers/usb/Makefile
>> @@ -6,6 +6,7 @@
>>
>> obj-$(CONFIG_USB) += core/
>> obj-$(CONFIG_USB_SUPPORT) += phy/
>> +obj-$(CONFIG_USB_SUPPORT) += mux/
>>
>> obj-$(CONFIG_USB_DWC3) += dwc3/
>> obj-$(CONFIG_USB_DWC2) += dwc2/
>> diff --git a/drivers/usb/mux/Kconfig b/drivers/usb/mux/Kconfig
>> new file mode 100644
>> index 0000000..8fedc4f
>> --- /dev/null
>> +++ b/drivers/usb/mux/Kconfig
>> @@ -0,0 +1,9 @@
>> +#
>> +# USB port mux driver configuration
>> +#
>> +menu "USB Port MUX drivers"
>> +config INTEL_USB_MUX
>> + select EXTCON
>> + def_bool n
> No help text?
>
>> +
>> +endmenu
>> diff --git a/drivers/usb/mux/Makefile b/drivers/usb/mux/Makefile
>> new file mode 100644
>> index 0000000..84f0ae8
>> --- /dev/null
>> +++ b/drivers/usb/mux/Makefile
>> @@ -0,0 +1,4 @@
>> +#
>> +# Makefile for USB port mux drivers
>> +#
>> +obj-$(CONFIG_INTEL_USB_MUX) += intel-mux.o
>> diff --git a/drivers/usb/mux/intel-mux.c b/drivers/usb/mux/intel-mux.c
>> new file mode 100644
>> index 0000000..b243758
>> --- /dev/null
>> +++ b/drivers/usb/mux/intel-mux.c
>> @@ -0,0 +1,166 @@
>> +/**
>> + * mux.c - USB Port Mux support
>> + *
>> + * Copyright (C) 2016 Intel Corporation
>> + *
>> + * Author: Lu Baolu <baolu.lu@...ux.intel.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +#include <linux/notifier.h>
>> +#include <linux/extcon.h>
>> +#include <linux/usb/intel-mux.h>
>> +#include <linux/err.h>
>> +
>> +struct intel_usb_mux {
>> + struct intel_mux_dev *umdev;
>> + struct notifier_block nb;
>> + struct extcon_specific_cable_nb obj;
>> +
>> + /*
>> + * The state of the mux.
>> + * 0, 1 - mux switch state
>> + * -1 - uninitialized state
>> + *
>> + * mux_mutex is lock to protect mux_state
>> + */
>> + int mux_state;
>> + struct mutex mux_mutex;
>> +};
>> +
>> +static int usb_mux_change_state(struct intel_usb_mux *mux, int state)
>> +{
>> + int ret;
>> + struct intel_mux_dev *umdev = mux->umdev;
>> +
>> + dev_WARN_ONCE(umdev->dev, !mutex_is_locked(&mux->mux_mutex),
>> + "mutex is unlocked\n");
>> +
>> + mux->mux_state = state;
>> +
>> + if (mux->mux_state)
>> + ret = umdev->cable_set_cb(umdev);
>> + else
>> + ret = umdev->cable_unset_cb(umdev);
>> +
>> + return ret;
>> +}
>> +
>> +static int usb_mux_notifier(struct notifier_block *nb,
>> + unsigned long event, void *ptr)
>> +{
>> + struct intel_usb_mux *mux;
>> + int state;
>> + int ret = NOTIFY_DONE;
>> +
>> + mux = container_of(nb, struct intel_usb_mux, nb);
>> +
>> + state = extcon_get_cable_state(mux->obj.edev,
>> + mux->umdev->cable_name);
>> +
>> + if (mux->mux_state == -1 || mux->mux_state != state) {
>> + mutex_lock(&mux->mux_mutex);
>> + ret = usb_mux_change_state(mux, state);
>> + mutex_unlock(&mux->mux_mutex);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static ssize_t intel_mux_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct intel_usb_mux *mux = dev_get_drvdata(dev);
>> +
>> + if (dev_WARN_ONCE(dev, !mux, "mux without data structure\n"))
>> + return 0;
>> +
>> + return sprintf(buf, "%s\n", mux->mux_state ? "host" : "peripheral");
>> +}
>> +
>> +static ssize_t intel_mux_store(struct device *dev,
>> + struct device_attribute *attr, const char *buf, size_t count)
>> +{
>> + struct intel_usb_mux *mux = dev_get_drvdata(dev);
>> + int state;
>> +
>> + if (dev_WARN_ONCE(dev, !mux, "mux without data structure\n"))
>> + return -EINVAL;
>> +
>> + if (sysfs_streq(buf, "peripheral"))
>> + state = 0;
>> + else if (sysfs_streq(buf, "host"))
>> + state = 1;
>> + else
>> + return -EINVAL;
>> +
>> + mutex_lock(&mux->mux_mutex);
>> + usb_mux_change_state(mux, state);
>> + mutex_unlock(&mux->mux_mutex);
>> +
>> + return count;
>> +}
>> +static DEVICE_ATTR_RW(intel_mux);
>> +
>> +int intel_usb_mux_register(struct intel_mux_dev *umdev)
>> +{
>> + int ret;
>> + struct device *dev = umdev->dev;
>> + struct intel_usb_mux *mux;
>> +
>> + if (!umdev->cable_name)
>> + return -ENODEV;
>> +
>> + mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
>> + if (!mux)
>> + return -ENOMEM;
>> +
>> + mux->umdev = umdev;
>> + mux->nb.notifier_call = usb_mux_notifier;
>> + mutex_init(&mux->mux_mutex);
>> + mux->mux_state = -1;
>> + dev_set_drvdata(dev, mux);
>> + ret = extcon_register_interest(&mux->obj, umdev->extcon_name,
>> + umdev->cable_name, &mux->nb);
>> + if (ret) {
>> + dev_err(dev, "failed to register extcon notifier\n");
>> + return -ENODEV;
>> + }
>> +
>> + usb_mux_notifier(&mux->nb, 0, NULL);
>> +
>> + /* register the sysfs interface */
>> + ret = device_create_file(dev, &dev_attr_intel_mux);
>> + if (ret) {
>> + dev_err(dev, "failed to create sysfs attribute\n");
>> + if (umdev->cable_name)
>> + extcon_unregister_interest(&mux->obj);
>> + return -ENODEV;
>> + }
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(intel_usb_mux_register);
>> +
>> +int intel_usb_mux_unregister(struct device *dev)
>> +{
>> + struct intel_usb_mux *mux = dev_get_drvdata(dev);
>> +
>> + device_remove_file(dev, &dev_attr_intel_mux);
>> + extcon_unregister_interest(&mux->obj);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(intel_usb_mux_unregister);
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +void intel_usb_mux_complete(struct device *dev)
>> +{
>> + struct intel_usb_mux *mux = dev_get_drvdata(dev);
>> +
>> + usb_mux_notifier(&mux->nb, 0, NULL);
>> +}
>> +EXPORT_SYMBOL_GPL(intel_usb_mux_complete);
>> +#endif
>> diff --git a/include/linux/usb/intel-mux.h b/include/linux/usb/intel-mux.h
>> new file mode 100644
>> index 0000000..6990174
>> --- /dev/null
>> +++ b/include/linux/usb/intel-mux.h
>> @@ -0,0 +1,47 @@
>> +/**
>> + * mux.h - USB Port Mux defines
>> + *
>> + * Copyright (C) 2016 Intel Corporation
>> + *
>> + * Author: Lu Baolu <baolu.lu@...ux.intel.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#ifndef __LINUX_USB_INTEL_MUX_H
>> +#define __LINUX_USB_INTEL_MUX_H
>> +
>> +#include <linux/usb.h>
>> +
>> +struct intel_mux_dev {
>> + struct device *dev;
>> + char *extcon_name;
>> + char *cable_name;
>> + int (*cable_set_cb)(struct intel_mux_dev *mux);
>> + int (*cable_unset_cb)(struct intel_mux_dev *mux);
>> +};
> This is a device, why not make it one? Don't just hold a reference.
> And do you really even hold that reference?
It's not a device. It's just an encapsulation for parameters passed into
intel_usb_mux_register().
>
>> +#if IS_ENABLED(CONFIG_INTEL_USB_MUX)
>> +extern int intel_usb_mux_register(struct intel_mux_dev *mux);
>> +extern int intel_usb_mux_unregister(struct device *dev);
> It's obvious you didn't run this through checkpatch.pl, please do so...
I did, but didn't hit any errors or warnings.
$ ./scripts/checkpatch.pl v3-0003-usb-mux-add-common-code-for-Intel-dual-role-port-.patch
total: 0 errors, 0 warnings, 272 lines checked
v3-0003-usb-mux-add-common-code-for-Intel-dual-role-port-.patch has no obvious style problems and is ready for submission.
> And your api is horrid, think about what you want the "core" to do here,
> it should be the one creating the device and returning it to the caller,
> not forcing the caller to somehow create it first and then pass it in.
This isn't a layer or core. It doesn't create any new devices. It's actually
some shared code which can be used by all Intel dual role port drivers.
I put it in a separated file because 1) this can avoid duplication; 2) this code
could be used for any architectures as long as a USB port is shared by
two components and it needs an OS response when event triggers.
I guess intel_usb_mux_register/unregister() is a bit misleading. How about
changing them to intel_usb_mux_probe/remove()?
> And why is it not symmetrical, you are passing one thing into register
> and another into unregister.
struct intel_mux_dev is an encapsulation for parameters passed into
intel_usb_mux_register(). It's not a new device structure though the name
is a bit misleading. How about remove this structure and put these in
function parameters?
>
> Come on, I expect better work from Intel kernel developers, I shouldn't
> have to be complaining about basic crap like this...
I am sorry to disappoint you.
Best Regards,
Baolu
>
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Powered by blists - more mailing lists