Re: [PATCH 8/8] xhci: Fix NULL pointer dereference in xhci_clear_tt_buffer_complete()
On 7.10.2019 17.02, Johan Hovold wrote:
[ +CC: Alan ]
On Fri, Oct 04, 2019 at 02:59:33PM +0300, Mathias Nyman wrote:
udev stored in ep->hcpriv might be NULL if tt buffer is cleared
due to a halted control endpoint during device enumeration
xhci_clear_tt_buffer_complete is called by hub_tt_work() once it's
scheduled, and by then usb core might have freed and allocated a
new udev for the next enumeration attempt.
Fixes: ef513be0a905 ("usb: xhci: Add Clear_TT_Buffer")
Cc: # v5.3
Reported-by: Johan Hovold
Signed-off-by: Mathias Nyman
---
drivers/usb/host/xhci.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 00f3804f7aa7..517ec3206f6e 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -5238,8 +5238,16 @@ static void xhci_clear_tt_buffer_complete(struct usb_hcd
*hcd,
unsigned int ep_index;
unsigned long flags;
+ /*
+* udev might be NULL if tt buffer is cleared during a failed device
+* enumeration due to a halted control endpoint. Usb core might
+* have allocated a new udev for the next enumeration attempt.
+*/
+
xhci = hcd_to_xhci(hcd);
udev = (struct usb_device *)ep->hcpriv;
+ if (!udev)
+ return;
I didn't have time to look into this myself last week, or comment on the
patch before Greg picked it up, but this clearly isn't the right fix.
As your comment suggests, ep->hcpriv may indeed be NULL here if USB core
have allocated a new udev. But this only happens after USB has freed the
old usb_device and the new one happens to get the same address.
You're right, that fix doesn't solve the actual issue, it avoids a few specific
null pointer dereference cases, but leaves both root cause and several other
use-after-free cases open.
Note that even the usb_host_endpoint itself (ep) has then been freed and
reallocated since it is member of struct usb_device, and it is the
use-after-free that needs fixing.
I've even been able to trigger another NULL-deref in this function
before a new udev has been allocated, due to the virt dev having been
freed by xhci_free_dev as part of usb_release_dev:
It seems the xhci clear-tt implementation was incomplete since it did
not take care to wait for any ongoing work before disabling the
endpoint. EHCI does this in ehci_endpoint_disable(), but xhci doesn't
even implement that callback.
So it seems, it might be possible to remove pending clear_tt work for
most endpoints in the .drop_endpoint callbacks, but ep0 is different,
it isn't dropped, we might need to implement the endpoint_disable()
callback for this.
As this may be something you could end up hitting in other paths as
well, perhaps we should even consider reverting the offending commit
pending a more complete implementation?
Possibly, if we can't find a working solution early enough in this release cycle
-Mathias
[PATCH] USB: core: drop OOM message
Drop redundant OOM message on allocation failures which would already
have been logged by the allocator. This also allows us to clean up the
error paths somewhat.
Signed-off-by: Johan Hovold
---
drivers/usb/core/config.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
index 151a74a54386..ff9f50f7218f 100644
--- a/drivers/usb/core/config.c
+++ b/drivers/usb/core/config.c
@@ -800,10 +800,10 @@ int usb_get_configuration(struct usb_device *dev)
{
struct device *ddev = &dev->dev;
int ncfg = dev->descriptor.bNumConfigurations;
- int result = -ENOMEM;
unsigned int cfgno, length;
unsigned char *bigbuffer;
struct usb_config_descriptor *desc;
+ int result;
if (ncfg > USB_MAXCONFIG) {
dev_warn(ddev, "too many configurations: %d, "
@@ -819,16 +819,16 @@ int usb_get_configuration(struct usb_device *dev)
length = ncfg * sizeof(struct usb_host_config);
dev->config = kzalloc(length, GFP_KERNEL);
if (!dev->config)
- goto err2;
+ return -ENOMEM;
length = ncfg * sizeof(char *);
dev->rawdescriptors = kzalloc(length, GFP_KERNEL);
if (!dev->rawdescriptors)
- goto err2;
+ return -ENOMEM;
desc = kmalloc(USB_DT_CONFIG_SIZE, GFP_KERNEL);
if (!desc)
- goto err2;
+ return -ENOMEM;
for (cfgno = 0; cfgno < ncfg; cfgno++) {
/* We grab just the first descriptor so we know how long
@@ -890,9 +890,7 @@ int usb_get_configuration(struct usb_device *dev)
err:
kfree(desc);
dev->descriptor.bNumConfigurations = cfgno;
-err2:
- if (result == -ENOMEM)
- dev_err(ddev, "out of memory\n");
+
return result;
}
--
2.23.0
[PATCH v3 1/9] usb: typec: Copy everything from struct typec_capability during registration
Copying everything from struct typec_capability to struct
typec_port during port registration. This will make sure
that under no circumstances the driver can change the values
in the struct typec_capability that the port uses.
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/class.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 94a3eda62add..0bbf10c8ad58 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -52,6 +52,7 @@ struct typec_port {
struct typec_switch *sw;
struct typec_mux*mux;
+ const struct typec_capability *orig_cap; /* to be removed */
const struct typec_capability *cap;
};
@@ -968,7 +969,7 @@ preferred_role_store(struct device *dev, struct
device_attribute *attr,
return -EINVAL;
}
- ret = port->cap->try_role(port->cap, role);
+ ret = port->cap->try_role(port->orig_cap, role);
if (ret)
return ret;
@@ -1014,7 +1015,7 @@ static ssize_t data_role_store(struct device *dev,
goto unlock_and_ret;
}
- ret = port->cap->dr_set(port->cap, ret);
+ ret = port->cap->dr_set(port->orig_cap, ret);
if (ret)
goto unlock_and_ret;
@@ -1071,7 +1072,7 @@ static ssize_t power_role_store(struct device *dev,
goto unlock_and_ret;
}
- ret = port->cap->pr_set(port->cap, ret);
+ ret = port->cap->pr_set(port->orig_cap, ret);
if (ret)
goto unlock_and_ret;
@@ -1119,7 +1120,7 @@ port_type_store(struct device *dev, struct
device_attribute *attr,
goto unlock_and_ret;
}
- ret = port->cap->port_type_set(port->cap, type);
+ ret = port->cap->port_type_set(port->orig_cap, type);
if (ret)
goto unlock_and_ret;
@@ -1184,7 +1185,7 @@ static ssize_t vconn_source_store(struct device *dev,
if (ret)
return ret;
- ret = port->cap->vconn_set(port->cap, (enum typec_role)source);
+ ret = port->cap->vconn_set(port->orig_cap, (enum typec_role)source);
if (ret)
return ret;
@@ -1278,6 +1279,7 @@ static void typec_release(struct device *dev)
ida_destroy(&port->mode_ids);
typec_switch_put(port->sw);
typec_mux_put(port->mux);
+ kfree(port->cap);
kfree(port);
}
@@ -1579,9 +1581,10 @@ struct typec_port *typec_register_port(struct device
*parent,
mutex_init(&port->port_type_lock);
port->id = id;
- port->cap = cap;
+ port->orig_cap = cap;
port->port_type = cap->type;
port->prefer_role = cap->prefer_role;
+ port->cap = kmemdup(cap, sizeof(*cap), GFP_KERNEL);
device_initialize(&port->dev);
port->dev.class = typec_class;
--
2.23.0
[PATCH v3 0/9] usb: typec: Small API improvement
Hi, The broken conditions in the *_store() functions should now be fixed. Cover letter from v2: In this version there should be no more semantic changes. The original cover letter: This series moves the callback members from struct typec_capabilities to a new struct typec_operations. That removes the need for the drivers to keep a copy of the struct typec_capabilites if they don't need it, and struct typec_operations can probable always be constified. The change is small, however I think the code ends up being a bit more cleaner looking this way. Let me know if it's OK. thanks, Heikki Krogerus (9): usb: typec: Copy everything from struct typec_capability during registration usb: typec: Introduce typec_get_drvdata() usb: typec: Separate the operations vector usb: typec: tcpm: Start using struct typec_operations usb: typec: tps6598x: Start using struct typec_operations usb: typec: ucsi: Start using struct typec_operations usb: typec: hd3ss3220: Start using struct typec_operations usb: typec: Remove the callback members from struct typec_capability usb: typec: Remove unused members from struct typec_capability drivers/usb/typec/class.c | 37 ++ drivers/usb/typec/hd3ss3220.c | 24 + drivers/usb/typec/tcpm/tcpm.c | 45 ++-- drivers/usb/typec/tps6598x.c | 49 +++ drivers/usb/typec/ucsi/ucsi.c | 22 include/linux/usb/typec.h | 41 - 6 files changed, 119 insertions(+), 99 deletions(-) -- 2.23.0
[PATCH v3 2/9] usb: typec: Introduce typec_get_drvdata()
Leaving the private driver_data pointer of the port device
to the port drivers.
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/class.c | 11 +++
include/linux/usb/typec.h | 4
2 files changed, 15 insertions(+)
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 0bbf10c8ad58..89ffe370e426 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -1488,6 +1488,16 @@ EXPORT_SYMBOL_GPL(typec_set_mode);
/* --- */
+/**
+ * typec_get_drvdata - Return private driver data pointer
+ * @port: USB Type-C port
+ */
+void *typec_get_drvdata(struct typec_port *port)
+{
+ return dev_get_drvdata(&port->dev);
+}
+EXPORT_SYMBOL_GPL(typec_get_drvdata);
+
/**
* typec_port_register_altmode - Register USB Type-C Port Alternate Mode
* @port: USB Type-C Port that supports the alternate mode
@@ -1592,6 +1602,7 @@ struct typec_port *typec_register_port(struct device
*parent,
port->dev.fwnode = cap->fwnode;
port->dev.type = &typec_port_dev_type;
dev_set_name(&port->dev, "port%d", id);
+ dev_set_drvdata(&port->dev, cap->driver_data);
port->sw = typec_switch_get(&port->dev);
if (IS_ERR(port->sw)) {
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index 7df4ecabc78a..8b90cd77331c 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -179,6 +179,7 @@ struct typec_partner_desc {
* @sw: Cable plug orientation switch
* @mux: Multiplexer switch for Alternate/Accessory Modes
* @fwnode: Optional fwnode of the port
+ * @driver_data: Private pointer for driver specific info
* @try_role: Set data role preference for DRP port
* @dr_set: Set Data Role
* @pr_set: Set Power Role
@@ -198,6 +199,7 @@ struct typec_capability {
struct typec_switch *sw;
struct typec_mux*mux;
struct fwnode_handle*fwnode;
+ void*driver_data;
int (*try_role)(const struct typec_capability *,
int role);
@@ -241,6 +243,8 @@ int typec_set_orientation(struct typec_port *port,
enum typec_orientation typec_get_orientation(struct typec_port *port);
int typec_set_mode(struct typec_port *port, int mode);
+void *typec_get_drvdata(struct typec_port *port);
+
int typec_find_port_power_role(const char *name);
int typec_find_power_role(const char *name);
int typec_find_port_data_role(const char *name);
--
2.23.0
[PATCH v3 3/9] usb: typec: Separate the operations vector
Introducing struct typec_operations which has the same
callbacks as struct typec_capability. The old callbacks are
kept for now, but after all users have been converted, they
will be removed.
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/class.c | 39 +--
include/linux/usb/typec.h | 20
2 files changed, 49 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 89ffe370e426..11ed3dc6fc49 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -54,6 +54,7 @@ struct typec_port {
const struct typec_capability *orig_cap; /* to be removed */
const struct typec_capability *cap;
+ const struct typec_operations *ops;
};
#define to_typec_port(_dev_) container_of(_dev_, struct typec_port, dev)
@@ -956,7 +957,7 @@ preferred_role_store(struct device *dev, struct
device_attribute *attr,
return -EOPNOTSUPP;
}
- if (!port->cap->try_role) {
+ if (!port->cap->try_role && (!port->ops || !port->ops->try_role)) {
dev_dbg(dev, "Setting preferred role not supported\n");
return -EOPNOTSUPP;
}
@@ -969,7 +970,10 @@ preferred_role_store(struct device *dev, struct
device_attribute *attr,
return -EINVAL;
}
- ret = port->cap->try_role(port->orig_cap, role);
+ if (port->ops && port->ops->try_role)
+ ret = port->ops->try_role(port, role);
+ else
+ ret = port->cap->try_role(port->orig_cap, role);
if (ret)
return ret;
@@ -1000,7 +1004,7 @@ static ssize_t data_role_store(struct device *dev,
struct typec_port *port = to_typec_port(dev);
int ret;
- if (!port->cap->dr_set) {
+ if (!port->cap->dr_set && (!port->ops || !port->ops->dr_set)) {
dev_dbg(dev, "data role swapping not supported\n");
return -EOPNOTSUPP;
}
@@ -1015,7 +1019,10 @@ static ssize_t data_role_store(struct device *dev,
goto unlock_and_ret;
}
- ret = port->cap->dr_set(port->orig_cap, ret);
+ if (port->ops && port->ops->dr_set)
+ ret = port->ops->dr_set(port, ret);
+ else
+ ret = port->cap->dr_set(port->orig_cap, ret);
if (ret)
goto unlock_and_ret;
@@ -1050,7 +1057,7 @@ static ssize_t power_role_store(struct device *dev,
return -EOPNOTSUPP;
}
- if (!port->cap->pr_set) {
+ if (!port->cap->pr_set && (!port->ops || !port->ops->pr_set)) {
dev_dbg(dev, "power role swapping not supported\n");
return -EOPNOTSUPP;
}
@@ -1072,7 +1079,10 @@ static ssize_t power_role_store(struct device *dev,
goto unlock_and_ret;
}
- ret = port->cap->pr_set(port->orig_cap, ret);
+ if (port->ops && port->ops->dr_set)
+ ret = port->ops->pr_set(port, ret);
+ else
+ ret = port->cap->pr_set(port->orig_cap, ret);
if (ret)
goto unlock_and_ret;
@@ -1103,7 +1113,8 @@ port_type_store(struct device *dev, struct
device_attribute *attr,
int ret;
enum typec_port_type type;
- if (!port->cap->port_type_set || port->cap->type != TYPEC_PORT_DRP) {
+ if (port->cap->type != TYPEC_PORT_DRP || (!port->cap->port_type_set &&
+ (!port->ops || !port->ops->port_type_set))) {
dev_dbg(dev, "changing port type not supported\n");
return -EOPNOTSUPP;
}
@@ -1120,7 +1131,10 @@ port_type_store(struct device *dev, struct
device_attribute *attr,
goto unlock_and_ret;
}
- ret = port->cap->port_type_set(port->orig_cap, type);
+ if (port->ops && port->ops->port_type_set)
+ ret = port->ops->port_type_set(port, type);
+ else
+ ret = port->cap->port_type_set(port->orig_cap, type);
if (ret)
goto unlock_and_ret;
@@ -1176,7 +1190,7 @@ static ssize_t vconn_source_store(struct device *dev,
return -EOPNOTSUPP;
}
- if (!port->cap->vconn_set) {
+ if (!port->cap->vconn_set && (!port->ops || !port->ops->vconn_set)) {
dev_dbg(dev, "VCONN swapping not supported\n");
return -EOPNOTSUPP;
}
@@ -1185,7 +1199,11 @@ static ssize_t vconn_source_store(struct device *dev,
if (ret)
return ret;
- ret = port->cap->vconn_set(port->orig_cap, (enum typec_role)source);
+ if (port->ops && port->ops->vconn_set)
+ ret = port->ops->vconn_set(port, (enum typec_role)source);
+ else
+ ret = port->cap->vconn_set(port->orig_cap,
+ (enum typec_role)source);
if (ret)
return ret;
@@ -1591,6 +1609,7 @@ struct typec_
[PATCH v3 6/9] usb: typec: ucsi: Start using struct typec_operations
Supplying the operation callbacks as part of a struct
typec_operations instead of as part of struct
typec_capability during port registration.
Signed-off-by: Heikki Krogerus
Reviewed-by: Guenter Roeck
---
drivers/usb/typec/ucsi/ucsi.c | 22 +++---
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index ba288b964dc8..edd722fb88b8 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -17,9 +17,6 @@
#include "ucsi.h"
#include "trace.h"
-#define to_ucsi_connector(_cap_) container_of(_cap_, struct ucsi_connector, \
- typec_cap)
-
/*
* UCSI_TIMEOUT_MS - PPM communication timeout
*
@@ -713,10 +710,9 @@ static int ucsi_role_cmd(struct ucsi_connector *con,
struct ucsi_control *ctrl)
return ret;
}
-static int
-ucsi_dr_swap(const struct typec_capability *cap, enum typec_data_role role)
+static int ucsi_dr_swap(struct typec_port *port, enum typec_data_role role)
{
- struct ucsi_connector *con = to_ucsi_connector(cap);
+ struct ucsi_connector *con = typec_get_drvdata(port);
struct ucsi_control ctrl;
int ret = 0;
@@ -748,10 +744,9 @@ ucsi_dr_swap(const struct typec_capability *cap, enum
typec_data_role role)
return ret < 0 ? ret : 0;
}
-static int
-ucsi_pr_swap(const struct typec_capability *cap, enum typec_role role)
+static int ucsi_pr_swap(struct typec_port *port, enum typec_role role)
{
- struct ucsi_connector *con = to_ucsi_connector(cap);
+ struct ucsi_connector *con = typec_get_drvdata(port);
struct ucsi_control ctrl;
int ret = 0;
@@ -788,6 +783,11 @@ ucsi_pr_swap(const struct typec_capability *cap, enum
typec_role role)
return ret;
}
+static const struct typec_operations ucsi_ops = {
+ .dr_set = ucsi_dr_swap,
+ .pr_set = ucsi_pr_swap
+};
+
static struct fwnode_handle *ucsi_find_fwnode(struct ucsi_connector *con)
{
struct fwnode_handle *fwnode;
@@ -843,8 +843,8 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
*accessory = TYPEC_ACCESSORY_DEBUG;
cap->fwnode = ucsi_find_fwnode(con);
- cap->dr_set = ucsi_dr_swap;
- cap->pr_set = ucsi_pr_swap;
+ cap->driver_data = con;
+ cap->ops = &ucsi_ops;
/* Register the connector */
con->port = typec_register_port(ucsi->dev, cap);
--
2.23.0
[PATCH v3 7/9] usb: typec: hd3ss3220: Start using struct typec_operations
Supplying the operation callbacks as part of a struct
typec_operations instead of as part of struct
typec_capability during port registration. After this there
is not need to keep the capabilities stored anywhere in the
driver.
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/hd3ss3220.c | 24 +---
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
index 1900910c637e..7c3ee9d28670 100644
--- a/drivers/usb/typec/hd3ss3220.c
+++ b/drivers/usb/typec/hd3ss3220.c
@@ -38,7 +38,6 @@ struct hd3ss3220 {
struct regmap *regmap;
struct usb_role_switch *role_sw;
struct typec_port *port;
- struct typec_capability typec_cap;
};
static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int src_pref)
@@ -74,11 +73,9 @@ static enum usb_role hd3ss3220_get_attached_state(struct
hd3ss3220 *hd3ss3220)
return attached_state;
}
-static int hd3ss3220_dr_set(const struct typec_capability *cap,
- enum typec_data_role role)
+static int hd3ss3220_dr_set(struct typec_port *port, enum typec_data_role role)
{
- struct hd3ss3220 *hd3ss3220 = container_of(cap, struct hd3ss3220,
- typec_cap);
+ struct hd3ss3220 *hd3ss3220 = typec_get_drvdata(port);
enum usb_role role_val;
int pref, ret = 0;
@@ -99,6 +96,10 @@ static int hd3ss3220_dr_set(const struct typec_capability
*cap,
return ret;
}
+static const struct typec_operations hd3ss3220_ops = {
+ .dr_set = hd3ss3220_dr_set
+};
+
static void hd3ss3220_set_role(struct hd3ss3220 *hd3ss3220)
{
enum usb_role role_state = hd3ss3220_get_attached_state(hd3ss3220);
@@ -153,6 +154,7 @@ static const struct regmap_config config = {
static int hd3ss3220_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
+ struct typec_capability typec_cap = { };
struct hd3ss3220 *hd3ss3220;
struct fwnode_handle *connector;
int ret;
@@ -181,13 +183,13 @@ static int hd3ss3220_probe(struct i2c_client *client,
if (IS_ERR_OR_NULL(hd3ss3220->role_sw))
return PTR_ERR(hd3ss3220->role_sw);
- hd3ss3220->typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
- hd3ss3220->typec_cap.dr_set = hd3ss3220_dr_set;
- hd3ss3220->typec_cap.type = TYPEC_PORT_DRP;
- hd3ss3220->typec_cap.data = TYPEC_PORT_DRD;
+ typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
+ typec_cap.driver_data = hd3ss3220;
+ typec_cap.type = TYPEC_PORT_DRP;
+ typec_cap.data = TYPEC_PORT_DRD;
+ typec_cap.ops = &hd3ss3220_ops;
- hd3ss3220->port = typec_register_port(&client->dev,
- &hd3ss3220->typec_cap);
+ hd3ss3220->port = typec_register_port(&client->dev, &typec_cap);
if (IS_ERR(hd3ss3220->port))
return PTR_ERR(hd3ss3220->port);
--
2.23.0
[PATCH v3 9/9] usb: typec: Remove unused members from struct typec_capability
The members for the muxes are not used, so dropping them.
Signed-off-by: Heikki Krogerus
---
include/linux/usb/typec.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index 894798084319..0f52723a11bd 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -209,8 +209,6 @@ struct typec_capability {
int prefer_role;
enum typec_accessoryaccessory[TYPEC_MAX_ACCESSORY];
- struct typec_switch *sw;
- struct typec_mux*mux;
struct fwnode_handle*fwnode;
void*driver_data;
--
2.23.0
[PATCH v3 4/9] usb: typec: tcpm: Start using struct typec_operations
Supplying the operation callbacks as part of a struct
typec_operations instead of as part of struct
typec_capability during port registration.
Signed-off-by: Heikki Krogerus
Reviewed-by: Guenter Roeck
---
drivers/usb/typec/tcpm/tcpm.c | 45 ---
1 file changed, 20 insertions(+), 25 deletions(-)
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 5f61d9977a15..bc9edb4c013b 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -390,12 +390,6 @@ static enum tcpm_state tcpm_default_state(struct tcpm_port
*port)
return SRC_UNATTACHED;
}
-static inline
-struct tcpm_port *typec_cap_to_tcpm(const struct typec_capability *cap)
-{
- return container_of(cap, struct tcpm_port, typec_caps);
-}
-
static bool tcpm_port_is_disconnected(struct tcpm_port *port)
{
return (!port->attached && port->cc1 == TYPEC_CC_OPEN &&
@@ -3970,10 +3964,9 @@ void tcpm_pd_hard_reset(struct tcpm_port *port)
}
EXPORT_SYMBOL_GPL(tcpm_pd_hard_reset);
-static int tcpm_dr_set(const struct typec_capability *cap,
- enum typec_data_role data)
+static int tcpm_dr_set(struct typec_port *p, enum typec_data_role data)
{
- struct tcpm_port *port = typec_cap_to_tcpm(cap);
+ struct tcpm_port *port = typec_get_drvdata(p);
int ret;
mutex_lock(&port->swap_lock);
@@ -4038,10 +4031,9 @@ static int tcpm_dr_set(const struct typec_capability
*cap,
return ret;
}
-static int tcpm_pr_set(const struct typec_capability *cap,
- enum typec_role role)
+static int tcpm_pr_set(struct typec_port *p, enum typec_role role)
{
- struct tcpm_port *port = typec_cap_to_tcpm(cap);
+ struct tcpm_port *port = typec_get_drvdata(p);
int ret;
mutex_lock(&port->swap_lock);
@@ -4082,10 +4074,9 @@ static int tcpm_pr_set(const struct typec_capability
*cap,
return ret;
}
-static int tcpm_vconn_set(const struct typec_capability *cap,
- enum typec_role role)
+static int tcpm_vconn_set(struct typec_port *p, enum typec_role role)
{
- struct tcpm_port *port = typec_cap_to_tcpm(cap);
+ struct tcpm_port *port = typec_get_drvdata(p);
int ret;
mutex_lock(&port->swap_lock);
@@ -4122,9 +4113,9 @@ static int tcpm_vconn_set(const struct typec_capability
*cap,
return ret;
}
-static int tcpm_try_role(const struct typec_capability *cap, int role)
+static int tcpm_try_role(struct typec_port *p, int role)
{
- struct tcpm_port *port = typec_cap_to_tcpm(cap);
+ struct tcpm_port *port = typec_get_drvdata(p);
struct tcpc_dev *tcpc = port->tcpc;
int ret = 0;
@@ -4331,10 +4322,9 @@ static void tcpm_init(struct tcpm_port *port)
tcpm_set_state(port, PORT_RESET, 0);
}
-static int tcpm_port_type_set(const struct typec_capability *cap,
- enum typec_port_type type)
+static int tcpm_port_type_set(struct typec_port *p, enum typec_port_type type)
{
- struct tcpm_port *port = typec_cap_to_tcpm(cap);
+ struct tcpm_port *port = typec_get_drvdata(p);
mutex_lock(&port->lock);
if (type == port->port_type)
@@ -4359,6 +4349,14 @@ static int tcpm_port_type_set(const struct
typec_capability *cap,
return 0;
}
+static const struct typec_operations tcpm_ops = {
+ .try_role = tcpm_try_role,
+ .dr_set = tcpm_dr_set,
+ .pr_set = tcpm_pr_set,
+ .vconn_set = tcpm_vconn_set,
+ .port_type_set = tcpm_port_type_set
+};
+
void tcpm_tcpc_reset(struct tcpm_port *port)
{
mutex_lock(&port->lock);
@@ -4772,11 +4770,8 @@ struct tcpm_port *tcpm_register_port(struct device *dev,
struct tcpc_dev *tcpc)
port->typec_caps.fwnode = tcpc->fwnode;
port->typec_caps.revision = 0x0120; /* Type-C spec release 1.2 */
port->typec_caps.pd_revision = 0x0300; /* USB-PD spec release 3.0 */
- port->typec_caps.dr_set = tcpm_dr_set;
- port->typec_caps.pr_set = tcpm_pr_set;
- port->typec_caps.vconn_set = tcpm_vconn_set;
- port->typec_caps.try_role = tcpm_try_role;
- port->typec_caps.port_type_set = tcpm_port_type_set;
+ port->typec_caps.driver_data = port;
+ port->typec_caps.ops = &tcpm_ops;
port->partner_desc.identity = &port->partner_ident;
port->port_type = port->typec_caps.type;
--
2.23.0
[PATCH v3 8/9] usb: typec: Remove the callback members from struct typec_capability
There are no more users for them.
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/class.c | 40 +++
include/linux/usb/typec.h | 17 -
2 files changed, 11 insertions(+), 46 deletions(-)
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 11ed3dc6fc49..3e9fa2530b86 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -52,7 +52,6 @@ struct typec_port {
struct typec_switch *sw;
struct typec_mux*mux;
- const struct typec_capability *orig_cap; /* to be removed */
const struct typec_capability *cap;
const struct typec_operations *ops;
};
@@ -957,7 +956,7 @@ preferred_role_store(struct device *dev, struct
device_attribute *attr,
return -EOPNOTSUPP;
}
- if (!port->cap->try_role && (!port->ops || !port->ops->try_role)) {
+ if (!port->ops || !port->ops->try_role) {
dev_dbg(dev, "Setting preferred role not supported\n");
return -EOPNOTSUPP;
}
@@ -970,10 +969,7 @@ preferred_role_store(struct device *dev, struct
device_attribute *attr,
return -EINVAL;
}
- if (port->ops && port->ops->try_role)
- ret = port->ops->try_role(port, role);
- else
- ret = port->cap->try_role(port->orig_cap, role);
+ ret = port->ops->try_role(port, role);
if (ret)
return ret;
@@ -1004,7 +1000,7 @@ static ssize_t data_role_store(struct device *dev,
struct typec_port *port = to_typec_port(dev);
int ret;
- if (!port->cap->dr_set && (!port->ops || !port->ops->dr_set)) {
+ if (!port->ops || !port->ops->dr_set) {
dev_dbg(dev, "data role swapping not supported\n");
return -EOPNOTSUPP;
}
@@ -1019,10 +1015,7 @@ static ssize_t data_role_store(struct device *dev,
goto unlock_and_ret;
}
- if (port->ops && port->ops->dr_set)
- ret = port->ops->dr_set(port, ret);
- else
- ret = port->cap->dr_set(port->orig_cap, ret);
+ ret = port->ops->dr_set(port, ret);
if (ret)
goto unlock_and_ret;
@@ -1057,7 +1050,7 @@ static ssize_t power_role_store(struct device *dev,
return -EOPNOTSUPP;
}
- if (!port->cap->pr_set && (!port->ops || !port->ops->pr_set)) {
+ if (!port->ops || !port->ops->pr_set) {
dev_dbg(dev, "power role swapping not supported\n");
return -EOPNOTSUPP;
}
@@ -1079,10 +1072,7 @@ static ssize_t power_role_store(struct device *dev,
goto unlock_and_ret;
}
- if (port->ops && port->ops->dr_set)
- ret = port->ops->pr_set(port, ret);
- else
- ret = port->cap->pr_set(port->orig_cap, ret);
+ ret = port->ops->pr_set(port, ret);
if (ret)
goto unlock_and_ret;
@@ -1113,8 +1103,8 @@ port_type_store(struct device *dev, struct
device_attribute *attr,
int ret;
enum typec_port_type type;
- if (port->cap->type != TYPEC_PORT_DRP || (!port->cap->port_type_set &&
- (!port->ops || !port->ops->port_type_set))) {
+ if (port->cap->type != TYPEC_PORT_DRP ||
+ !port->ops || !port->ops->port_type_set) {
dev_dbg(dev, "changing port type not supported\n");
return -EOPNOTSUPP;
}
@@ -1131,10 +1121,7 @@ port_type_store(struct device *dev, struct
device_attribute *attr,
goto unlock_and_ret;
}
- if (port->ops && port->ops->port_type_set)
- ret = port->ops->port_type_set(port, type);
- else
- ret = port->cap->port_type_set(port->orig_cap, type);
+ ret = port->ops->port_type_set(port, type);
if (ret)
goto unlock_and_ret;
@@ -1190,7 +1177,7 @@ static ssize_t vconn_source_store(struct device *dev,
return -EOPNOTSUPP;
}
- if (!port->cap->vconn_set && (!port->ops || !port->ops->vconn_set)) {
+ if (!port->ops || !port->ops->vconn_set) {
dev_dbg(dev, "VCONN swapping not supported\n");
return -EOPNOTSUPP;
}
@@ -1199,11 +1186,7 @@ static ssize_t vconn_source_store(struct device *dev,
if (ret)
return ret;
- if (port->ops && port->ops->vconn_set)
- ret = port->ops->vconn_set(port, (enum typec_role)source);
- else
- ret = port->cap->vconn_set(port->orig_cap,
- (enum typec_role)source);
+ ret = port->ops->vconn_set(port, (enum typec_role)source);
if (ret)
return ret;
@@ -1610,7 +1593,6 @@ struct typec_port *typec_register_port(struct device
*parent,
port->id = id;
port->ops = cap->ops;
- p
[PATCH v3 5/9] usb: typec: tps6598x: Start using struct typec_operations
Supplying the operation callbacks as part of a struct
typec_operations instead of as part of struct
typec_capability during port registration. After this there
is not need to keep the capabilities stored anywhere in the
driver.
Signed-off-by: Heikki Krogerus
Reviewed-by: Guenter Roeck
---
drivers/usb/typec/tps6598x.c | 49 +++-
1 file changed, 26 insertions(+), 23 deletions(-)
diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
index a38d1409f15b..0698addd1185 100644
--- a/drivers/usb/typec/tps6598x.c
+++ b/drivers/usb/typec/tps6598x.c
@@ -94,7 +94,6 @@ struct tps6598x {
struct typec_port *port;
struct typec_partner *partner;
struct usb_pd_identity partner_identity;
- struct typec_capability typec_cap;
};
/*
@@ -307,11 +306,10 @@ static int tps6598x_exec_cmd(struct tps6598x *tps, const
char *cmd,
return 0;
}
-static int
-tps6598x_dr_set(const struct typec_capability *cap, enum typec_data_role role)
+static int tps6598x_dr_set(struct typec_port *port, enum typec_data_role role)
{
- struct tps6598x *tps = container_of(cap, struct tps6598x, typec_cap);
const char *cmd = (role == TYPEC_DEVICE) ? "SWUF" : "SWDF";
+ struct tps6598x *tps = typec_get_drvdata(port);
u32 status;
int ret;
@@ -338,11 +336,10 @@ tps6598x_dr_set(const struct typec_capability *cap, enum
typec_data_role role)
return ret;
}
-static int
-tps6598x_pr_set(const struct typec_capability *cap, enum typec_role role)
+static int tps6598x_pr_set(struct typec_port *port, enum typec_role role)
{
- struct tps6598x *tps = container_of(cap, struct tps6598x, typec_cap);
const char *cmd = (role == TYPEC_SINK) ? "SWSk" : "SWSr";
+ struct tps6598x *tps = typec_get_drvdata(port);
u32 status;
int ret;
@@ -369,6 +366,11 @@ tps6598x_pr_set(const struct typec_capability *cap, enum
typec_role role)
return ret;
}
+static const struct typec_operations tps6598x_ops = {
+ .dr_set = tps6598x_dr_set,
+ .pr_set = tps6598x_pr_set,
+};
+
static irqreturn_t tps6598x_interrupt(int irq, void *data)
{
struct tps6598x *tps = data;
@@ -448,6 +450,7 @@ static const struct regmap_config tps6598x_regmap_config = {
static int tps6598x_probe(struct i2c_client *client)
{
+ struct typec_capability typec_cap = { };
struct tps6598x *tps;
u32 status;
u32 conf;
@@ -492,40 +495,40 @@ static int tps6598x_probe(struct i2c_client *client)
if (ret < 0)
return ret;
- tps->typec_cap.revision = USB_TYPEC_REV_1_2;
- tps->typec_cap.pd_revision = 0x200;
- tps->typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
- tps->typec_cap.pr_set = tps6598x_pr_set;
- tps->typec_cap.dr_set = tps6598x_dr_set;
+ typec_cap.revision = USB_TYPEC_REV_1_2;
+ typec_cap.pd_revision = 0x200;
+ typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
+ typec_cap.driver_data = tps;
+ typec_cap.ops = &tps6598x_ops;
switch (TPS_SYSCONF_PORTINFO(conf)) {
case TPS_PORTINFO_SINK_ACCESSORY:
case TPS_PORTINFO_SINK:
- tps->typec_cap.type = TYPEC_PORT_SNK;
- tps->typec_cap.data = TYPEC_PORT_UFP;
+ typec_cap.type = TYPEC_PORT_SNK;
+ typec_cap.data = TYPEC_PORT_UFP;
break;
case TPS_PORTINFO_DRP_UFP_DRD:
case TPS_PORTINFO_DRP_DFP_DRD:
- tps->typec_cap.type = TYPEC_PORT_DRP;
- tps->typec_cap.data = TYPEC_PORT_DRD;
+ typec_cap.type = TYPEC_PORT_DRP;
+ typec_cap.data = TYPEC_PORT_DRD;
break;
case TPS_PORTINFO_DRP_UFP:
- tps->typec_cap.type = TYPEC_PORT_DRP;
- tps->typec_cap.data = TYPEC_PORT_UFP;
+ typec_cap.type = TYPEC_PORT_DRP;
+ typec_cap.data = TYPEC_PORT_UFP;
break;
case TPS_PORTINFO_DRP_DFP:
- tps->typec_cap.type = TYPEC_PORT_DRP;
- tps->typec_cap.data = TYPEC_PORT_DFP;
+ typec_cap.type = TYPEC_PORT_DRP;
+ typec_cap.data = TYPEC_PORT_DFP;
break;
case TPS_PORTINFO_SOURCE:
- tps->typec_cap.type = TYPEC_PORT_SRC;
- tps->typec_cap.data = TYPEC_PORT_DFP;
+ typec_cap.type = TYPEC_PORT_SRC;
+ typec_cap.data = TYPEC_PORT_DFP;
break;
default:
return -ENODEV;
}
- tps->port = typec_register_port(&client->dev, &tps->typec_cap);
+ tps->port = typec_register_port(&client->dev, &typec_cap);
if (IS_ERR(tps->port))
return PTR_ERR(tps->port);
--
2.23.0
Re: [PATCH v4] usb: hub: Check device descriptor before resusciation
On Tue, Oct 08, 2019 at 10:09:01AM +0200, David Heinzelmann wrote: > If a device connected to an xHCI host controller disconnects from the USB bus > and then reconnects, e.g. triggered by a firmware update, then the host > controller automatically activates the connection and the port is enabled. The > implementation of hub_port_connect_change() assumes that if the port is > enabled then nothing has changed. There is no check if the USB descriptors > have changed. As a result, the kernel's internal copy of the descriptors ends > up being incorrect and the device doesn't work properly anymore. > > The solution to the problem is for hub_port_connect_change() always to > check whether the device's descriptors have changed before resuscitating > an enabled port. > > Signed-off-by: David Heinzelmann > --- > Changes in v4: > - changed commit description > Changes in v3: > - changed commit message and description > - fix code style > Changes in v2: > - fix logic error to handle return code from usb_get_device_descriptor() >properly > - fix line endings > --- > drivers/usb/core/hub.c | 196 +++-- > 1 file changed, 111 insertions(+), 85 deletions(-) What happened to Alan's ack? v5? thanks, greg k-h
Re: [PATCH v4] usb: hub: Check device descriptor before resusciation
On Tue, Oct 08, 2019 at 02:55:46PM +0200, Greg KH wrote: > What happened to Alan's ack? > I'm not sure I'm allowed to add someone else's acked-by tag? If so I will sent v5. David
Re: [PATCH v4] usb: hub: Check device descriptor before resusciation
On Tue, Oct 08, 2019 at 06:10:22PM +0200, David Heinzelmann wrote: > On Tue, Oct 08, 2019 at 02:55:46PM +0200, Greg KH wrote: > > What happened to Alan's ack? > > > > I'm not sure I'm allowed to add someone else's acked-by tag? If they provide it, and you don't change anything, then yes, you can. > If so I will sent v5. Please do. thanks, greg k-h
Re: CREAD ignored by almost all USB serial drivers
On Mon, Oct 07, 2019 at 03:18:52PM +0200, Harald Welte wrote: > Hi Greg, > > On Mon, Oct 07, 2019 at 01:06:33PM +0200, Greg KH wrote: > > On Sat, Sep 28, 2019 at 10:49:55PM +0200, Harald Welte wrote: > > > It seems that a lot of Linux kernel USB serial device drivers are > > > ignoring the CREAD setting of termios.c_cflag. > > > > You just discovered something that has been broken since the first > > usb-serial driver was written, all those years ago :) > > Amazing and frightening at the same time. I would have expected > somebody had built something like a hardware test fixture to test those > drivers during all those decades. Something like a "well-known" serial > device as the tester, attaching to all the handshake etc. lines of the > "device under test" and then running through many of the possible > settings from HW to SW flow control, baud rate, parity, number of stop > bits, break characters, etc. I had a tester that was just a loopback device. Amazing what just a simple device like that found over time, these things barely work :) Anyway, yes, a "real" test setup like this would be great to have, I don't know of one around anywhere, and if you use it, I am sure you will find lots of issues. Turns out almost all usb-serial devices are used for "basic" rx/tx stuff, all of the "fancy" serial things just are not all that common anymore given that serial is not the primary way data is transferred anymore. > I have no shortage of projects to work on, but if somebody else was > interested to host a physical setup with many different [USB] serial > ports and some CI around, I might be tempted to build the actual tester > hardware and some test suite software for it. Would be nice to see :) > > I did add support for this to the digi driver, as you saw, as the > > hardware had support for it. For everything else, they are all just > > dumb uarts and do not expose that information to the host computer and I > > think everyone just forgot about this option. > > I am aware that many USB serial adapters are rather "dumb", hence my > suggestion > to add a related option to the core usb-serial, or even to the core tty/serial > layer: If the driver doesn't process CREAD, simply discard the received bytes > at this common/shared layer. Yes, that could be done in the usb-serial core probably. > > Given that you are the first to report it that I can think of, I don't > > think very many people use half-duplex protocols with a shared Rx/Tx > > (which is crazy anyway...) > > Every smart card interface [1] on this planet, including every SIM card in > every > mobile phone uses such a setup: asynchronous half-duplex communication with > shared Rx/Tx. Sure, not many people attach something like that to a > USB-Serial > adapter (as oppose to a USB-CCID reader), but I just wanted to clarify > it's not as obscure as one may think. You can actually buy > ultra-low-cost SIM card readers built that way. > > Also, I would assume that RS-485 is still used in lots of technology, > including e.g. DMX and industrial control systems. Unless you go for a > rather obscure 4-wire RS-485, then you have the same half-duplex > operation on shared medium. Please note that USB-RS485 adapters exist, > using a variety of USB-serial chipsets. 485 just got added to some tty drivers recently, so yes, it is used, but not all that common it seems. Or maybe it is and everyone "knows" to buy the one good device that supports it. thanks, greg k-h
Re: [PATCH] usb: dwc3: dwc3-meson-g12a.c: use devm_platform_ioremap_resource()
On Tue, Oct 08, 2019 at 07:29:28PM +0200, Martin Blumenstingl wrote: > Hi Saurav, > > On Tue, Oct 8, 2019 at 5:06 PM Kevin Hilman wrote: > > > > Saurav Girepunje writes: > > > > > Use the new helper that wraps the calls to platform_get_resource() > > > and devm_ioremap_resource() together in dwc3_meson_g12a_probe(). > > > > > > Signed-off-by: Saurav Girepunje > the following commit is already in mainline: > > commit c6e4999cd930b8bd11dd8d4767e871b47f502845 > Author: YueHaibing > Date: Fri Aug 2 21:04:08 2019 +0800 >usb: dwc3: meson-g12a: use devm_platform_ioremap_resource() to simplify > code > > > Martin Ok...Thanks for the information.
[PATCH] usbfs: Suppress uevents for claiminterface/releaseinterface
Hello,
With recent Ubuntu 18/Linux Mint 19.2 etc, lots of user space programs
(in particular systemd/eudev/upowerd) have problems with the "BIND/UNBIND"
events produced since kernel 4.13.
Some problems are described, when googling for
linux "usb" "bind event"
Now this might be blamed on these particular user space programs.
But: This also means that programs accessing a USB device via the generic
usbfs layer can easily flood the kernel and all user space programs listening
to uevents with tons of BIND/UNBIND events by calling
ioctl(usbfd, USBDEVFS_CLAIMINTERFACE, &intf);
ioctl(usbfd, USBDEVFS_RELEASEINTERFACE, &intf);
in a tight loop.
Of course this is an extreme example, but I have a use case where exactly
this happens (running Linux Mint 19.2).
The result is that "systemd-udev" needs > 100% CPU and
upowerd spams the system log with messages about "bind/unbind" events.
I am also not sure if these particular bind/unbind events contain any useful
information; these events just mean an arbitrary user space program has
bound/unbound from a particular USB interface.
The following patch tries to suppress emission of uevents
for USB interfaces which are claimed/released via usbfs.
I am not sure if this is the right way to do it, but at least
it seems to do what I intended...
with best regards
Ingo Rohloff
From 57970b0a5a36809ddb8f15687c18ca2147dc73bd Mon Sep 17 00:00:00 2001
From: Ingo Rohloff
Date: Tue, 8 Oct 2019 20:27:57 +0200
Subject: [PATCH] USB: usbfs: Suppress emission of uevents for interfaces
handled via usbfs.
commit 1455cf8dbfd0
("driver core: emit uevents when device is bound to a driver")
added BIND and UNBIND events when a driver is bound/unbound
to a physical device.
For USB devices which are handled via the generic usbfs layer
(via libusb for example). This is problematic:
Each time a user space program calls
ioctl(usb_fd, USBDEVFS_CLAIMINTERFACE, &usb_intf_nr);
and then later
ioctl(usb_fd, USBDEVFS_RELEASEINTERFACE, &usb_intf_nr);
The kernel will now produce a BIND/UNBIND event, which
does not really contain any useful information.
Additionally this easily allows a user space program to run a
DoS attack against programs which listen to uevents
(in particular systemd/eudev/upowerd):
A malicious user space program just has to call in a tight loop
ioctl(usbfd, USBDEVFS_CLAIMINTERFACE, &intf);
ioctl(usbfd, USBDEVFS_RELEASEINTERFACE, &intf);
with this loop the malicious user space program floods
the kernel and all programs listening to uevents with
tons of BIND/UNBIND events.
The following patch tries to suppress uevents for interfaces
claimed via usbfs.
---
drivers/usb/core/devio.c | 7 ++-
drivers/usb/core/driver.c | 2 ++
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 3f899552f6e3..a1af1d9b2ae7 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -764,8 +764,13 @@ static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
intf = usb_ifnum_to_if(dev, ifnum);
if (!intf)
err = -ENOENT;
- else
+ else {
+ /* suppress uevents for devices handled by usbfs */
+ dev_set_uevent_suppress(&intf->dev, 1);
err = usb_driver_claim_interface(&usbfs_driver, intf, ps);
+ if (err != 0)
+ dev_set_uevent_suppress(&intf->dev, 0);
+ }
if (err == 0)
set_bit(ifnum, &ps->ifclaimed);
return err;
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 2b27d232d7a7..6a15bc5c2869 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -594,6 +594,8 @@ void usb_driver_release_interface(struct usb_driver *driver,
*/
if (device_is_registered(dev)) {
device_release_driver(dev);
+ /* make sure we allow uevents again */
+ dev_set_uevent_suppress(dev, 0);
} else {
device_lock(dev);
usb_unbind_interface(dev);
--
2.17.1
Re: [PATCH] usbfs: Suppress uevents for claiminterface/releaseinterface
On Tue, Oct 08, 2019 at 09:32:10PM +0200, Ingo Rohloff wrote:
> Hello,
>
> With recent Ubuntu 18/Linux Mint 19.2 etc, lots of user space programs
> (in particular systemd/eudev/upowerd) have problems with the "BIND/UNBIND"
> events produced since kernel 4.13.
> Some problems are described, when googling for
> linux "usb" "bind event"
>
> Now this might be blamed on these particular user space programs.
> But: This also means that programs accessing a USB device via the generic
> usbfs layer can easily flood the kernel and all user space programs listening
> to uevents with tons of BIND/UNBIND events by calling
>
> ioctl(usbfd, USBDEVFS_CLAIMINTERFACE, &intf);
> ioctl(usbfd, USBDEVFS_RELEASEINTERFACE, &intf);
>
> in a tight loop.
> Of course this is an extreme example, but I have a use case where exactly
> this happens (running Linux Mint 19.2).
> The result is that "systemd-udev" needs > 100% CPU and
> upowerd spams the system log with messages about "bind/unbind" events.
>
> I am also not sure if these particular bind/unbind events contain any useful
> information; these events just mean an arbitrary user space program has
> bound/unbound from a particular USB interface.
>
> The following patch tries to suppress emission of uevents
> for USB interfaces which are claimed/released via usbfs.
>
> I am not sure if this is the right way to do it, but at least
> it seems to do what I intended...
>
> with best regards
> Ingo Rohloff
> From 57970b0a5a36809ddb8f15687c18ca2147dc73bd Mon Sep 17 00:00:00 2001
> From: Ingo Rohloff
> Date: Tue, 8 Oct 2019 20:27:57 +0200
> Subject: [PATCH] USB: usbfs: Suppress emission of uevents for interfaces
> handled via usbfs.
>
> commit 1455cf8dbfd0
> ("driver core: emit uevents when device is bound to a driver")
> added BIND and UNBIND events when a driver is bound/unbound
> to a physical device.
>
> For USB devices which are handled via the generic usbfs layer
> (via libusb for example). This is problematic:
> Each time a user space program calls
>ioctl(usb_fd, USBDEVFS_CLAIMINTERFACE, &usb_intf_nr);
> and then later
>ioctl(usb_fd, USBDEVFS_RELEASEINTERFACE, &usb_intf_nr);
> The kernel will now produce a BIND/UNBIND event, which
> does not really contain any useful information.
>
> Additionally this easily allows a user space program to run a
> DoS attack against programs which listen to uevents
> (in particular systemd/eudev/upowerd):
> A malicious user space program just has to call in a tight loop
>
> ioctl(usbfd, USBDEVFS_CLAIMINTERFACE, &intf);
> ioctl(usbfd, USBDEVFS_RELEASEINTERFACE, &intf);
>
> with this loop the malicious user space program floods
> the kernel and all programs listening to uevents with
> tons of BIND/UNBIND events.
>
> The following patch tries to suppress uevents for interfaces
> claimed via usbfs.
> ---
> drivers/usb/core/devio.c | 7 ++-
> drivers/usb/core/driver.c | 2 ++
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 3f899552f6e3..a1af1d9b2ae7 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -764,8 +764,13 @@ static int claimintf(struct usb_dev_state *ps, unsigned
> int ifnum)
> intf = usb_ifnum_to_if(dev, ifnum);
> if (!intf)
> err = -ENOENT;
> - else
> + else {
> + /* suppress uevents for devices handled by usbfs */
> + dev_set_uevent_suppress(&intf->dev, 1);
> err = usb_driver_claim_interface(&usbfs_driver, intf, ps);
> + if (err != 0)
> + dev_set_uevent_suppress(&intf->dev, 0);
> + }
> if (err == 0)
> set_bit(ifnum, &ps->ifclaimed);
> return err;
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index 2b27d232d7a7..6a15bc5c2869 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -594,6 +594,8 @@ void usb_driver_release_interface(struct usb_driver
> *driver,
>*/
> if (device_is_registered(dev)) {
> device_release_driver(dev);
> + /* make sure we allow uevents again */
> + dev_set_uevent_suppress(dev, 0);
> } else {
> device_lock(dev);
> usb_unbind_interface(dev);
> --
> 2.17.1
>
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- Your patch was attached, please place it inline so that it can be
applied directly from the email mess
Unknown symbol errors in usb storage driver
Since a while I see the following. I didn't bisect yet, maybe issue is caused by
32bca2df7da2 ("usb-storage: export symbols in USB_STORAGE namespace")?
DEPMOD 5.4.0-rc2-next-20191008+
depmod: WARNING:
/lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
needs unknown symbol usb_stor_probe1
depmod: WARNING:
/lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
needs unknown symbol usb_stor_reset_resume
depmod: WARNING:
/lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
needs unknown symbol usb_stor_pre_reset
depmod: WARNING:
/lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
needs unknown symbol usb_stor_host_template_init
depmod: WARNING:
/lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
needs unknown symbol usb_stor_probe2
depmod: WARNING:
/lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
needs unknown symbol usb_stor_disconnect
depmod: WARNING:
/lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
needs unknown symbol usb_stor_control_msg
depmod: WARNING:
/lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
needs unknown symbol usb_stor_post_reset
depmod: WARNING:
/lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
needs unknown symbol usb_stor_bulk_transfer_buf
#
# Automatically generated file; DO NOT EDIT.
# Linux/x86 5.4.0-rc2 Kernel Configuration
#
#
# Compiler: gcc (GCC) 9.2.0
#
CONFIG_CC_IS_GCC=y
CONFIG_GCC_VERSION=90200
CONFIG_CLANG_VERSION=0
CONFIG_CC_CAN_LINK=y
CONFIG_CC_HAS_ASM_GOTO=y
CONFIG_CC_HAS_ASM_INLINE=y
CONFIG_CC_HAS_WARN_MAYBE_UNINITIALIZED=y
CONFIG_IRQ_WORK=y
CONFIG_BUILDTIME_EXTABLE_SORT=y
CONFIG_THREAD_INFO_IN_TASK=y
#
# General setup
#
CONFIG_INIT_ENV_ARG_LIMIT=32
# CONFIG_COMPILE_TEST is not set
CONFIG_HEADER_TEST=y
# CONFIG_KERNEL_HEADER_TEST is not set
CONFIG_LOCALVERSION=""
# CONFIG_LOCALVERSION_AUTO is not set
CONFIG_BUILD_SALT=""
CONFIG_HAVE_KERNEL_GZIP=y
CONFIG_HAVE_KERNEL_BZIP2=y
CONFIG_HAVE_KERNEL_LZMA=y
CONFIG_HAVE_KERNEL_XZ=y
CONFIG_HAVE_KERNEL_LZO=y
CONFIG_HAVE_KERNEL_LZ4=y
CONFIG_KERNEL_GZIP=y
# CONFIG_KERNEL_BZIP2 is not set
# CONFIG_KERNEL_LZMA is not set
# CONFIG_KERNEL_XZ is not set
# CONFIG_KERNEL_LZO is not set
# CONFIG_KERNEL_LZ4 is not set
CONFIG_DEFAULT_HOSTNAME="(none)"
CONFIG_SWAP=y
CONFIG_SYSVIPC=y
CONFIG_SYSVIPC_SYSCTL=y
CONFIG_POSIX_MQUEUE=y
CONFIG_POSIX_MQUEUE_SYSCTL=y
CONFIG_CROSS_MEMORY_ATTACH=y
CONFIG_USELIB=y
# CONFIG_AUDIT is not set
CONFIG_HAVE_ARCH_AUDITSYSCALL=y
#
# IRQ subsystem
#
CONFIG_GENERIC_IRQ_PROBE=y
CONFIG_GENERIC_IRQ_SHOW=y
CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK=y
CONFIG_GENERIC_PENDING_IRQ=y
CONFIG_GENERIC_IRQ_MIGRATION=y
CONFIG_IRQ_DOMAIN=y
CONFIG_IRQ_DOMAIN_HIERARCHY=y
CONFIG_GENERIC_MSI_IRQ=y
CONFIG_GENERIC_MSI_IRQ_DOMAIN=y
CONFIG_GENERIC_IRQ_MATRIX_ALLOCATOR=y
CONFIG_GENERIC_IRQ_RESERVATION_MODE=y
CONFIG_IRQ_FORCED_THREADING=y
CONFIG_SPARSE_IRQ=y
# CONFIG_GENERIC_IRQ_DEBUGFS is not set
# end of IRQ subsystem
CONFIG_CLOCKSOURCE_WATCHDOG=y
CONFIG_ARCH_CLOCKSOURCE_DATA=y
CONFIG_ARCH_CLOCKSOURCE_INIT=y
CONFIG_CLOCKSOURCE_VALIDATE_LAST_CYCLE=y
CONFIG_GENERIC_TIME_VSYSCALL=y
CONFIG_GENERIC_CLOCKEVENTS=y
CONFIG_GENERIC_CLOCKEVENTS_BROADCAST=y
CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST=y
CONFIG_GENERIC_CMOS_UPDATE=y
#
# Timers subsystem
#
CONFIG_TICK_ONESHOT=y
CONFIG_NO_HZ_COMMON=y
# CONFIG_HZ_PERIODIC is not set
CONFIG_NO_HZ_IDLE=y
# CONFIG_NO_HZ_FULL is not set
# CONFIG_NO_HZ is not set
CONFIG_HIGH_RES_TIMERS=y
# end of Timers subsystem
CONFIG_PREEMPT_NONE=y
# CONFIG_PREEMPT_VOLUNTARY is not set
# CONFIG_PREEMPT is not set
CONFIG_PREEMPT_COUNT=y
#
# CPU/Task time and stats accounting
#
CONFIG_TICK_CPU_ACCOUNTING=y
# CONFIG_VIRT_CPU_ACCOUNTING_GEN is not set
# CONFIG_IRQ_TIME_ACCOUNTING is not set
CONFIG_BSD_PROCESS_ACCT=y
# CONFIG_BSD_PROCESS_ACCT_V3 is not set
CONFIG_TASKSTATS=y
CONFIG_TASK_DELAY_ACCT=y
CONFIG_TASK_XACCT=y
CONFIG_TASK_IO_ACCOUNTING=y
# CONFIG_PSI is not set
# end of CPU/Task time and stats accounting
# CONFIG_CPU_ISOLATION is not set
#
# RCU Subsystem
#
CONFIG_TREE_RCU=y
# CONFIG_RCU_EXPERT is not set
CONFIG_SRCU=y
CONFIG_TREE_SRCU=y
CONFIG_RCU_STALL_COMMON=y
CONFIG_RCU_NEED_SEGCBLIST=y
# end of RCU Subsystem
CONFIG_IKCONFIG=m
CONFIG_IKCONFIG_PROC=y
# CONFIG_IKHEADERS is not set
CONFIG_LOG_BUF_SHIFT=18
CONFIG_LOG_CPU_MAX_BUF_SHIFT=12
CONFIG_PRINTK_SAFE_LOG_BUF_SHIFT=13
CONFIG_HAVE_UNSTABLE_SCHED_CLOCK=y
#
# Scheduler features
#
# end of Scheduler features
CONFIG_ARCH_SUPPORTS_NUMA_BALANCING=y
CONFIG_ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH=y
CONFIG_ARCH_SUPPORTS_INT128=y
CONFIG_CGROUPS=y
# CONFIG_MEMCG is not set
# CONFIG_BLK_CGROUP is not set
CONFIG_CGROUP_SCHED=y
CONFIG_FAIR_GROUP_SCHED=y
# CONFIG_CFS_BANDWIDTH is not set
# CONFIG_RT_GROUP_SCHED is not set
# CONFIG_CGROUP_PIDS is not set
# CONFIG_C
HERE IS YOUR MONEY GRAM PAYMENT HAS BEEN SENT TO YOU HERE IS THE M.T.C.N:78393135
HERE IS YOUR MONEY GRAM PAYMENT HAS BEEN SENT TO YOU HERE IS THE M.T.C.N:78393135 Attn: Beneficiary, This is to inform you that the America Embassy office was instructed to transfer your fund $980,000.00 U.S Dollars compensating all the SCAM VICTIMS and your email was found as one of the VICTIMS. by America security leading team and America representative officers so between today the 8th of October till 1ST Of December 2019 you will be receiving MONEY GRAM the sum of $6,000 dollars per day. However be informed that we have already sent the $6,000 dollars this morning to avoid cancellation of your payment, remain the total sum of $980,000.00. You have only six hours to call this office upon the receipt of this email the maximum amount you will be receiving per a day starting from today's $6,000 and the Money Transfer Control Number of today is below. NOTE; The sent $6,000 is on hold because of the instruction from IMF office, they asked us to place it on hold by requesting the (Clean Bill Record Certificate) which will cost you $25 in order to fulfill all the necessary obligation to avoid any hitches while sending you the payment through MONEY GRAM money transfer, the necessary obligation I mean here is to obtain the (Clean Bill Record Certificate) Below is the information of today track it in our websitehttps://moneygarm.com/asp/orderStatus.asp?country=global to see is available to pick up by the receiver, but if we didn't here from you soon we'll pickup it up from line for security reason to avoid hackers stealing the money online. Money Transfer Control Number M.T.C.N)::78393135 SENDERS FIRST NAME: John SENDERS LAST NAME: Chun SENDERS COUNTRY...BENIN REPUBLIC TEXT QUESTION: A ANSWER: B AMOUNT: $6,000 We need the below details from you, to enable us place the payment to your name and transfer the fund to you. (Full Receivers name)... (You're Country) (Address).. (Phone NuMBER-... (You're Age) (OCCUPATION)..REAL ESTATE.. (A Copy of Your ID CARD).SEE ATTACHMENTS. HOWEVER YOU HAVE TO PAY $25 FOR THE (Clean Bill Record Certificate) AND THAT IS ALL YOU HAVE TO DO ASAP. The payment will be sending to below information, such as: Receiver.. ALAN UDE CountryBenin Republic Amount: $25 Question: .A Answer:... B Sender...Name: MTCN :.. According to the instruction and order we received from IMF the their requested $25 must be made directly to the above info's. Furthermore you are advised to call us as the instruction was passed that within 6hours without hearing from you, Count your payment canceled. Number to call is below listed manager director office of release order: DR.ALAN UDE Director MONEY GRAM-Benin
Re: Unknown symbol errors in usb storage driver
On Tue, Oct 08, 2019 at 09:53:16PM +0200, Heiner Kallweit wrote:
> Since a while I see the following. I didn't bisect yet, maybe issue is caused
> by
> 32bca2df7da2 ("usb-storage: export symbols in USB_STORAGE namespace")?
>
> DEPMOD 5.4.0-rc2-next-20191008+
> depmod: WARNING:
> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
> needs unknown symbol usb_stor_probe1
> depmod: WARNING:
> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
> needs unknown symbol usb_stor_reset_resume
> depmod: WARNING:
> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
> needs unknown symbol usb_stor_pre_reset
> depmod: WARNING:
> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
> needs unknown symbol usb_stor_host_template_init
> depmod: WARNING:
> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
> needs unknown symbol usb_stor_probe2
> depmod: WARNING:
> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
> needs unknown symbol usb_stor_disconnect
> depmod: WARNING:
> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
> needs unknown symbol usb_stor_control_msg
> depmod: WARNING:
> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
> needs unknown symbol usb_stor_post_reset
> depmod: WARNING:
> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
> needs unknown symbol usb_stor_bulk_transfer_buf
>
It's a depmod bug, see lkml for the discussion and potential fixes.
People are working on it :)
thanks,
greg k-h
Re: Unknown symbol errors in usb storage driver
On 08.10.2019 22:05, Greg KH wrote:
> On Tue, Oct 08, 2019 at 09:53:16PM +0200, Heiner Kallweit wrote:
>> Since a while I see the following. I didn't bisect yet, maybe issue is
>> caused by
>> 32bca2df7da2 ("usb-storage: export symbols in USB_STORAGE namespace")?
>>
>> DEPMOD 5.4.0-rc2-next-20191008+
>> depmod: WARNING:
>> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
>> needs unknown symbol usb_stor_probe1
>> depmod: WARNING:
>> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
>> needs unknown symbol usb_stor_reset_resume
>> depmod: WARNING:
>> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
>> needs unknown symbol usb_stor_pre_reset
>> depmod: WARNING:
>> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
>> needs unknown symbol usb_stor_host_template_init
>> depmod: WARNING:
>> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
>> needs unknown symbol usb_stor_probe2
>> depmod: WARNING:
>> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
>> needs unknown symbol usb_stor_disconnect
>> depmod: WARNING:
>> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
>> needs unknown symbol usb_stor_control_msg
>> depmod: WARNING:
>> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
>> needs unknown symbol usb_stor_post_reset
>> depmod: WARNING:
>> /lib/modules/5.4.0-rc2-next-20191008+/kernel/drivers/usb/storage/ums-realtek.ko
>> needs unknown symbol usb_stor_bulk_transfer_buf
>>
>
>
> It's a depmod bug, see lkml for the discussion and potential fixes.
> People are working on it :)
>
Good to know, thanks for the quick reply.
> thanks,
>
> greg k-h
>
Heiner
Re: [PATCH v3 1/9] usb: typec: Copy everything from struct typec_capability during registration
On Tue, Oct 08, 2019 at 02:13:42PM +0300, Heikki Krogerus wrote:
> Copying everything from struct typec_capability to struct
> typec_port during port registration. This will make sure
> that under no circumstances the driver can change the values
> in the struct typec_capability that the port uses.
>
> Signed-off-by: Heikki Krogerus
> ---
> drivers/usb/typec/class.c | 15 +--
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 94a3eda62add..0bbf10c8ad58 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -52,6 +52,7 @@ struct typec_port {
> struct typec_switch *sw;
> struct typec_mux*mux;
>
> + const struct typec_capability *orig_cap; /* to be removed */
> const struct typec_capability *cap;
> };
>
> @@ -968,7 +969,7 @@ preferred_role_store(struct device *dev, struct
> device_attribute *attr,
> return -EINVAL;
> }
>
> - ret = port->cap->try_role(port->cap, role);
> + ret = port->cap->try_role(port->orig_cap, role);
> if (ret)
> return ret;
>
> @@ -1014,7 +1015,7 @@ static ssize_t data_role_store(struct device *dev,
> goto unlock_and_ret;
> }
>
> - ret = port->cap->dr_set(port->cap, ret);
> + ret = port->cap->dr_set(port->orig_cap, ret);
> if (ret)
> goto unlock_and_ret;
>
> @@ -1071,7 +1072,7 @@ static ssize_t power_role_store(struct device *dev,
> goto unlock_and_ret;
> }
>
> - ret = port->cap->pr_set(port->cap, ret);
> + ret = port->cap->pr_set(port->orig_cap, ret);
> if (ret)
> goto unlock_and_ret;
>
> @@ -1119,7 +1120,7 @@ port_type_store(struct device *dev, struct
> device_attribute *attr,
> goto unlock_and_ret;
> }
>
> - ret = port->cap->port_type_set(port->cap, type);
> + ret = port->cap->port_type_set(port->orig_cap, type);
> if (ret)
> goto unlock_and_ret;
>
> @@ -1184,7 +1185,7 @@ static ssize_t vconn_source_store(struct device *dev,
> if (ret)
> return ret;
>
> - ret = port->cap->vconn_set(port->cap, (enum typec_role)source);
> + ret = port->cap->vconn_set(port->orig_cap, (enum typec_role)source);
> if (ret)
> return ret;
>
> @@ -1278,6 +1279,7 @@ static void typec_release(struct device *dev)
> ida_destroy(&port->mode_ids);
> typec_switch_put(port->sw);
> typec_mux_put(port->mux);
> + kfree(port->cap);
> kfree(port);
> }
>
> @@ -1579,9 +1581,10 @@ struct typec_port *typec_register_port(struct device
> *parent,
> mutex_init(&port->port_type_lock);
>
> port->id = id;
> - port->cap = cap;
> + port->orig_cap = cap;
> port->port_type = cap->type;
> port->prefer_role = cap->prefer_role;
> + port->cap = kmemdup(cap, sizeof(*cap), GFP_KERNEL);
I just realized ... unfortunately kmemdup() can return NULL.
>
> device_initialize(&port->dev);
> port->dev.class = typec_class;
> --
> 2.23.0
>
Re: [PATCH v3 2/9] usb: typec: Introduce typec_get_drvdata()
On Tue, Oct 08, 2019 at 02:13:43PM +0300, Heikki Krogerus wrote:
> Leaving the private driver_data pointer of the port device
> to the port drivers.
>
> Signed-off-by: Heikki Krogerus
Reviewed-by: Guenter Roeck
> ---
> drivers/usb/typec/class.c | 11 +++
> include/linux/usb/typec.h | 4
> 2 files changed, 15 insertions(+)
>
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 0bbf10c8ad58..89ffe370e426 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -1488,6 +1488,16 @@ EXPORT_SYMBOL_GPL(typec_set_mode);
>
> /* --- */
>
> +/**
> + * typec_get_drvdata - Return private driver data pointer
> + * @port: USB Type-C port
> + */
> +void *typec_get_drvdata(struct typec_port *port)
> +{
> + return dev_get_drvdata(&port->dev);
> +}
> +EXPORT_SYMBOL_GPL(typec_get_drvdata);
> +
> /**
> * typec_port_register_altmode - Register USB Type-C Port Alternate Mode
> * @port: USB Type-C Port that supports the alternate mode
> @@ -1592,6 +1602,7 @@ struct typec_port *typec_register_port(struct device
> *parent,
> port->dev.fwnode = cap->fwnode;
> port->dev.type = &typec_port_dev_type;
> dev_set_name(&port->dev, "port%d", id);
> + dev_set_drvdata(&port->dev, cap->driver_data);
>
> port->sw = typec_switch_get(&port->dev);
> if (IS_ERR(port->sw)) {
> diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> index 7df4ecabc78a..8b90cd77331c 100644
> --- a/include/linux/usb/typec.h
> +++ b/include/linux/usb/typec.h
> @@ -179,6 +179,7 @@ struct typec_partner_desc {
> * @sw: Cable plug orientation switch
> * @mux: Multiplexer switch for Alternate/Accessory Modes
> * @fwnode: Optional fwnode of the port
> + * @driver_data: Private pointer for driver specific info
> * @try_role: Set data role preference for DRP port
> * @dr_set: Set Data Role
> * @pr_set: Set Power Role
> @@ -198,6 +199,7 @@ struct typec_capability {
> struct typec_switch *sw;
> struct typec_mux*mux;
> struct fwnode_handle*fwnode;
> + void*driver_data;
>
> int (*try_role)(const struct typec_capability *,
> int role);
> @@ -241,6 +243,8 @@ int typec_set_orientation(struct typec_port *port,
> enum typec_orientation typec_get_orientation(struct typec_port *port);
> int typec_set_mode(struct typec_port *port, int mode);
>
> +void *typec_get_drvdata(struct typec_port *port);
> +
> int typec_find_port_power_role(const char *name);
> int typec_find_power_role(const char *name);
> int typec_find_port_data_role(const char *name);
> --
> 2.23.0
>
Re: [PATCH v3 3/9] usb: typec: Separate the operations vector
On Tue, Oct 08, 2019 at 02:13:44PM +0300, Heikki Krogerus wrote:
> Introducing struct typec_operations which has the same
> callbacks as struct typec_capability. The old callbacks are
> kept for now, but after all users have been converted, they
> will be removed.
>
> Signed-off-by: Heikki Krogerus
Reviewed-by: Guenter Roeck
> ---
> drivers/usb/typec/class.c | 39 +--
> include/linux/usb/typec.h | 20
> 2 files changed, 49 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 89ffe370e426..11ed3dc6fc49 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -54,6 +54,7 @@ struct typec_port {
>
> const struct typec_capability *orig_cap; /* to be removed */
> const struct typec_capability *cap;
> + const struct typec_operations *ops;
> };
>
> #define to_typec_port(_dev_) container_of(_dev_, struct typec_port, dev)
> @@ -956,7 +957,7 @@ preferred_role_store(struct device *dev, struct
> device_attribute *attr,
> return -EOPNOTSUPP;
> }
>
> - if (!port->cap->try_role) {
> + if (!port->cap->try_role && (!port->ops || !port->ops->try_role)) {
> dev_dbg(dev, "Setting preferred role not supported\n");
> return -EOPNOTSUPP;
> }
> @@ -969,7 +970,10 @@ preferred_role_store(struct device *dev, struct
> device_attribute *attr,
> return -EINVAL;
> }
>
> - ret = port->cap->try_role(port->orig_cap, role);
> + if (port->ops && port->ops->try_role)
> + ret = port->ops->try_role(port, role);
> + else
> + ret = port->cap->try_role(port->orig_cap, role);
> if (ret)
> return ret;
>
> @@ -1000,7 +1004,7 @@ static ssize_t data_role_store(struct device *dev,
> struct typec_port *port = to_typec_port(dev);
> int ret;
>
> - if (!port->cap->dr_set) {
> + if (!port->cap->dr_set && (!port->ops || !port->ops->dr_set)) {
> dev_dbg(dev, "data role swapping not supported\n");
> return -EOPNOTSUPP;
> }
> @@ -1015,7 +1019,10 @@ static ssize_t data_role_store(struct device *dev,
> goto unlock_and_ret;
> }
>
> - ret = port->cap->dr_set(port->orig_cap, ret);
> + if (port->ops && port->ops->dr_set)
> + ret = port->ops->dr_set(port, ret);
> + else
> + ret = port->cap->dr_set(port->orig_cap, ret);
> if (ret)
> goto unlock_and_ret;
>
> @@ -1050,7 +1057,7 @@ static ssize_t power_role_store(struct device *dev,
> return -EOPNOTSUPP;
> }
>
> - if (!port->cap->pr_set) {
> + if (!port->cap->pr_set && (!port->ops || !port->ops->pr_set)) {
> dev_dbg(dev, "power role swapping not supported\n");
> return -EOPNOTSUPP;
> }
> @@ -1072,7 +1079,10 @@ static ssize_t power_role_store(struct device *dev,
> goto unlock_and_ret;
> }
>
> - ret = port->cap->pr_set(port->orig_cap, ret);
> + if (port->ops && port->ops->dr_set)
> + ret = port->ops->pr_set(port, ret);
> + else
> + ret = port->cap->pr_set(port->orig_cap, ret);
> if (ret)
> goto unlock_and_ret;
>
> @@ -1103,7 +1113,8 @@ port_type_store(struct device *dev, struct
> device_attribute *attr,
> int ret;
> enum typec_port_type type;
>
> - if (!port->cap->port_type_set || port->cap->type != TYPEC_PORT_DRP) {
> + if (port->cap->type != TYPEC_PORT_DRP || (!port->cap->port_type_set &&
> + (!port->ops || !port->ops->port_type_set))) {
> dev_dbg(dev, "changing port type not supported\n");
> return -EOPNOTSUPP;
> }
> @@ -1120,7 +1131,10 @@ port_type_store(struct device *dev, struct
> device_attribute *attr,
> goto unlock_and_ret;
> }
>
> - ret = port->cap->port_type_set(port->orig_cap, type);
> + if (port->ops && port->ops->port_type_set)
> + ret = port->ops->port_type_set(port, type);
> + else
> + ret = port->cap->port_type_set(port->orig_cap, type);
> if (ret)
> goto unlock_and_ret;
>
> @@ -1176,7 +1190,7 @@ static ssize_t vconn_source_store(struct device *dev,
> return -EOPNOTSUPP;
> }
>
> - if (!port->cap->vconn_set) {
> + if (!port->cap->vconn_set && (!port->ops || !port->ops->vconn_set)) {
> dev_dbg(dev, "VCONN swapping not supported\n");
> return -EOPNOTSUPP;
> }
> @@ -1185,7 +1199,11 @@ static ssize_t vconn_source_store(struct device *dev,
> if (ret)
> return ret;
>
> - ret = port->cap->vconn_set(port->orig_cap, (enum typec_role)source);
> + if (port->ops && port->ops->vconn_set)
> + ret = port->ops->vconn_set(port, (enum typec_role)source);
> + else
> + ret
Re: [PATCH v3 7/9] usb: typec: hd3ss3220: Start using struct typec_operations
On Tue, Oct 08, 2019 at 02:13:48PM +0300, Heikki Krogerus wrote:
> Supplying the operation callbacks as part of a struct
> typec_operations instead of as part of struct
> typec_capability during port registration. After this there
> is not need to keep the capabilities stored anywhere in the
> driver.
>
> Signed-off-by: Heikki Krogerus
Reviewed-by: Guenter Roeck
> ---
> drivers/usb/typec/hd3ss3220.c | 24 +---
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
> index 1900910c637e..7c3ee9d28670 100644
> --- a/drivers/usb/typec/hd3ss3220.c
> +++ b/drivers/usb/typec/hd3ss3220.c
> @@ -38,7 +38,6 @@ struct hd3ss3220 {
> struct regmap *regmap;
> struct usb_role_switch *role_sw;
> struct typec_port *port;
> - struct typec_capability typec_cap;
> };
>
> static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int
> src_pref)
> @@ -74,11 +73,9 @@ static enum usb_role hd3ss3220_get_attached_state(struct
> hd3ss3220 *hd3ss3220)
> return attached_state;
> }
>
> -static int hd3ss3220_dr_set(const struct typec_capability *cap,
> - enum typec_data_role role)
> +static int hd3ss3220_dr_set(struct typec_port *port, enum typec_data_role
> role)
> {
> - struct hd3ss3220 *hd3ss3220 = container_of(cap, struct hd3ss3220,
> -typec_cap);
> + struct hd3ss3220 *hd3ss3220 = typec_get_drvdata(port);
> enum usb_role role_val;
> int pref, ret = 0;
>
> @@ -99,6 +96,10 @@ static int hd3ss3220_dr_set(const struct typec_capability
> *cap,
> return ret;
> }
>
> +static const struct typec_operations hd3ss3220_ops = {
> + .dr_set = hd3ss3220_dr_set
> +};
> +
> static void hd3ss3220_set_role(struct hd3ss3220 *hd3ss3220)
> {
> enum usb_role role_state = hd3ss3220_get_attached_state(hd3ss3220);
> @@ -153,6 +154,7 @@ static const struct regmap_config config = {
> static int hd3ss3220_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> + struct typec_capability typec_cap = { };
> struct hd3ss3220 *hd3ss3220;
> struct fwnode_handle *connector;
> int ret;
> @@ -181,13 +183,13 @@ static int hd3ss3220_probe(struct i2c_client *client,
> if (IS_ERR_OR_NULL(hd3ss3220->role_sw))
> return PTR_ERR(hd3ss3220->role_sw);
>
> - hd3ss3220->typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
> - hd3ss3220->typec_cap.dr_set = hd3ss3220_dr_set;
> - hd3ss3220->typec_cap.type = TYPEC_PORT_DRP;
> - hd3ss3220->typec_cap.data = TYPEC_PORT_DRD;
> + typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
> + typec_cap.driver_data = hd3ss3220;
> + typec_cap.type = TYPEC_PORT_DRP;
> + typec_cap.data = TYPEC_PORT_DRD;
> + typec_cap.ops = &hd3ss3220_ops;
>
> - hd3ss3220->port = typec_register_port(&client->dev,
> - &hd3ss3220->typec_cap);
> + hd3ss3220->port = typec_register_port(&client->dev, &typec_cap);
> if (IS_ERR(hd3ss3220->port))
> return PTR_ERR(hd3ss3220->port);
>
> --
> 2.23.0
>
Re: [PATCH v3 8/9] usb: typec: Remove the callback members from struct typec_capability
On Tue, Oct 08, 2019 at 02:13:49PM +0300, Heikki Krogerus wrote:
> There are no more users for them.
>
> Signed-off-by: Heikki Krogerus
Reviewed-by: Guenter Roeck
> ---
> drivers/usb/typec/class.c | 40 +++
> include/linux/usb/typec.h | 17 -
> 2 files changed, 11 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 11ed3dc6fc49..3e9fa2530b86 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -52,7 +52,6 @@ struct typec_port {
> struct typec_switch *sw;
> struct typec_mux*mux;
>
> - const struct typec_capability *orig_cap; /* to be removed */
> const struct typec_capability *cap;
> const struct typec_operations *ops;
> };
> @@ -957,7 +956,7 @@ preferred_role_store(struct device *dev, struct
> device_attribute *attr,
> return -EOPNOTSUPP;
> }
>
> - if (!port->cap->try_role && (!port->ops || !port->ops->try_role)) {
> + if (!port->ops || !port->ops->try_role) {
> dev_dbg(dev, "Setting preferred role not supported\n");
> return -EOPNOTSUPP;
> }
> @@ -970,10 +969,7 @@ preferred_role_store(struct device *dev, struct
> device_attribute *attr,
> return -EINVAL;
> }
>
> - if (port->ops && port->ops->try_role)
> - ret = port->ops->try_role(port, role);
> - else
> - ret = port->cap->try_role(port->orig_cap, role);
> + ret = port->ops->try_role(port, role);
> if (ret)
> return ret;
>
> @@ -1004,7 +1000,7 @@ static ssize_t data_role_store(struct device *dev,
> struct typec_port *port = to_typec_port(dev);
> int ret;
>
> - if (!port->cap->dr_set && (!port->ops || !port->ops->dr_set)) {
> + if (!port->ops || !port->ops->dr_set) {
> dev_dbg(dev, "data role swapping not supported\n");
> return -EOPNOTSUPP;
> }
> @@ -1019,10 +1015,7 @@ static ssize_t data_role_store(struct device *dev,
> goto unlock_and_ret;
> }
>
> - if (port->ops && port->ops->dr_set)
> - ret = port->ops->dr_set(port, ret);
> - else
> - ret = port->cap->dr_set(port->orig_cap, ret);
> + ret = port->ops->dr_set(port, ret);
> if (ret)
> goto unlock_and_ret;
>
> @@ -1057,7 +1050,7 @@ static ssize_t power_role_store(struct device *dev,
> return -EOPNOTSUPP;
> }
>
> - if (!port->cap->pr_set && (!port->ops || !port->ops->pr_set)) {
> + if (!port->ops || !port->ops->pr_set) {
> dev_dbg(dev, "power role swapping not supported\n");
> return -EOPNOTSUPP;
> }
> @@ -1079,10 +1072,7 @@ static ssize_t power_role_store(struct device *dev,
> goto unlock_and_ret;
> }
>
> - if (port->ops && port->ops->dr_set)
> - ret = port->ops->pr_set(port, ret);
> - else
> - ret = port->cap->pr_set(port->orig_cap, ret);
> + ret = port->ops->pr_set(port, ret);
> if (ret)
> goto unlock_and_ret;
>
> @@ -1113,8 +1103,8 @@ port_type_store(struct device *dev, struct
> device_attribute *attr,
> int ret;
> enum typec_port_type type;
>
> - if (port->cap->type != TYPEC_PORT_DRP || (!port->cap->port_type_set &&
> - (!port->ops || !port->ops->port_type_set))) {
> + if (port->cap->type != TYPEC_PORT_DRP ||
> + !port->ops || !port->ops->port_type_set) {
> dev_dbg(dev, "changing port type not supported\n");
> return -EOPNOTSUPP;
> }
> @@ -1131,10 +1121,7 @@ port_type_store(struct device *dev, struct
> device_attribute *attr,
> goto unlock_and_ret;
> }
>
> - if (port->ops && port->ops->port_type_set)
> - ret = port->ops->port_type_set(port, type);
> - else
> - ret = port->cap->port_type_set(port->orig_cap, type);
> + ret = port->ops->port_type_set(port, type);
> if (ret)
> goto unlock_and_ret;
>
> @@ -1190,7 +1177,7 @@ static ssize_t vconn_source_store(struct device *dev,
> return -EOPNOTSUPP;
> }
>
> - if (!port->cap->vconn_set && (!port->ops || !port->ops->vconn_set)) {
> + if (!port->ops || !port->ops->vconn_set) {
> dev_dbg(dev, "VCONN swapping not supported\n");
> return -EOPNOTSUPP;
> }
> @@ -1199,11 +1186,7 @@ static ssize_t vconn_source_store(struct device *dev,
> if (ret)
> return ret;
>
> - if (port->ops && port->ops->vconn_set)
> - ret = port->ops->vconn_set(port, (enum typec_role)source);
> - else
> - ret = port->cap->vconn_set(port->orig_cap,
> -(enum typec_role)source);
> + ret = port->ops->vconn_set(port, (enum typec_role)source);
> if (ret
Re: [PATCH v3 9/9] usb: typec: Remove unused members from struct typec_capability
On Tue, Oct 08, 2019 at 02:13:50PM +0300, Heikki Krogerus wrote:
> The members for the muxes are not used, so dropping them.
>
> Signed-off-by: Heikki Krogerus
Reviewed-by: Guenter Roeck
> ---
> include/linux/usb/typec.h | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> index 894798084319..0f52723a11bd 100644
> --- a/include/linux/usb/typec.h
> +++ b/include/linux/usb/typec.h
> @@ -209,8 +209,6 @@ struct typec_capability {
> int prefer_role;
> enum typec_accessoryaccessory[TYPEC_MAX_ACCESSORY];
>
> - struct typec_switch *sw;
> - struct typec_mux*mux;
> struct fwnode_handle*fwnode;
> void*driver_data;
>
> --
> 2.23.0
>
[PATCH] usb: chipidea: imx: check data->usbmisc_data against NULL before access
From: Li Jun
As usbmisc_data is optional, so add the check before access its member,
this fix below static checker warning:
drivers/usb/chipidea/ci_hdrc_imx.c:438 ci_hdrc_imx_probe()
warn: 'data->usbmisc_data' can also be NULL
which is introduced by Patch 15b80f7c3a7f:
"usb: chipidea: imx: enable vbus and id wakeup only for OTG events"
Reported-by: Dan Carpenter
Signed-off-by: Li Jun
---
drivers/usb/chipidea/ci_hdrc_imx.c | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
b/drivers/usb/chipidea/ci_hdrc_imx.c
index b11d70f..0498210 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -433,13 +433,15 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
goto err_clk;
}
- if (!IS_ERR(pdata.id_extcon.edev) ||
- of_property_read_bool(np, "usb-role-switch"))
- data->usbmisc_data->ext_id = 1;
-
- if (!IS_ERR(pdata.vbus_extcon.edev) ||
- of_property_read_bool(np, "usb-role-switch"))
- data->usbmisc_data->ext_vbus = 1;
+ if (data->usbmisc_data) {
+ if (!IS_ERR(pdata.id_extcon.edev) ||
+ of_property_read_bool(np, "usb-role-switch"))
+ data->usbmisc_data->ext_id = 1;
+
+ if (!IS_ERR(pdata.vbus_extcon.edev) ||
+ of_property_read_bool(np, "usb-role-switch"))
+ data->usbmisc_data->ext_vbus = 1;
+ }
ret = imx_usbmisc_init_post(data->usbmisc_data);
if (ret) {
--
2.7.4
[PATCH v5] usb: hub: Check device descriptor before resusciation
If a device connected to an xHCI host controller disconnects from the USB bus
and then reconnects, e.g. triggered by a firmware update, then the host
controller automatically activates the connection and the port is enabled. The
implementation of hub_port_connect_change() assumes that if the port is
enabled then nothing has changed. There is no check if the USB descriptors
have changed. As a result, the kernel's internal copy of the descriptors ends
up being incorrect and the device doesn't work properly anymore.
The solution to the problem is for hub_port_connect_change() always to
check whether the device's descriptors have changed before resuscitating
an enabled port.
Signed-off-by: David Heinzelmann
Acked-by: Alan Stern
---
Changes in v5:
- added Acked-by
Changes in v4:
- changed commit description
Changes in v3:
- changed commit message and description
- fix code style
Changes in v2:
- fix logic error to handle return code from usb_get_device_descriptor()
properly
- fix line endings
---
drivers/usb/core/hub.c | 196 +++--
1 file changed, 111 insertions(+), 85 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 236313f41f4a..fdcfa85b5b12 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4930,6 +4930,91 @@ hub_power_remaining(struct usb_hub *hub)
return remaining;
}
+
+static int descriptors_changed(struct usb_device *udev,
+ struct usb_device_descriptor *old_device_descriptor,
+ struct usb_host_bos *old_bos)
+{
+ int changed = 0;
+ unsignedindex;
+ unsignedserial_len = 0;
+ unsignedlen;
+ unsignedold_length;
+ int length;
+ char*buf;
+
+ if (memcmp(&udev->descriptor, old_device_descriptor,
+ sizeof(*old_device_descriptor)) != 0)
+ return 1;
+
+ if ((old_bos && !udev->bos) || (!old_bos && udev->bos))
+ return 1;
+ if (udev->bos) {
+ len = le16_to_cpu(udev->bos->desc->wTotalLength);
+ if (len != le16_to_cpu(old_bos->desc->wTotalLength))
+ return 1;
+ if (memcmp(udev->bos->desc, old_bos->desc, len))
+ return 1;
+ }
+
+ /* Since the idVendor, idProduct, and bcdDevice values in the
+* device descriptor haven't changed, we will assume the
+* Manufacturer and Product strings haven't changed either.
+* But the SerialNumber string could be different (e.g., a
+* different flash card of the same brand).
+*/
+ if (udev->serial)
+ serial_len = strlen(udev->serial) + 1;
+
+ len = serial_len;
+ for (index = 0; index < udev->descriptor.bNumConfigurations; index++) {
+ old_length = le16_to_cpu(udev->config[index].desc.wTotalLength);
+ len = max(len, old_length);
+ }
+
+ buf = kmalloc(len, GFP_NOIO);
+ if (!buf)
+ /* assume the worst */
+ return 1;
+
+ for (index = 0; index < udev->descriptor.bNumConfigurations; index++) {
+ old_length = le16_to_cpu(udev->config[index].desc.wTotalLength);
+ length = usb_get_descriptor(udev, USB_DT_CONFIG, index, buf,
+ old_length);
+ if (length != old_length) {
+ dev_dbg(&udev->dev, "config index %d, error %d\n",
+ index, length);
+ changed = 1;
+ break;
+ }
+ if (memcmp(buf, udev->rawdescriptors[index], old_length)
+ != 0) {
+ dev_dbg(&udev->dev, "config index %d changed (#%d)\n",
+ index,
+ ((struct usb_config_descriptor *) buf)->
+ bConfigurationValue);
+ changed = 1;
+ break;
+ }
+ }
+
+ if (!changed && serial_len) {
+ length = usb_string(udev, udev->descriptor.iSerialNumber,
+ buf, serial_len);
+ if (length + 1 != serial_len) {
+ dev_dbg(&udev->dev, "serial string error %d\n",
+ length);
+ changed = 1;
+ } else if (memcmp(buf, udev->serial, length) != 0) {
+ dev_dbg(&udev->dev, "serial string changed\n");
+ changed = 1;
+ }
+ }
+
+ kfree(buf);
+ return changed;
+}
+
static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
u16 portchange)
{
@@ -5167,7 +5252,9 @@ static void hub_port_connect_change(struct usb_hub *hub,
int port1,
{
struct us
