| Message ID | 20191217123345.31850-4-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 210e9f56e9e12472741b949950f9efcebf350750 |
| Headers | show |
| Series | thunderbolt: Add support for USB4 | expand |
On Tue, Dec 17, 2019 at 03:33:39PM +0300, Mika Westerberg wrote: > USB4 1.0 section 6.4.2.7 specifies a new field (PG) in notification > packet that is sent as response of hot plug/unplug events. This field > tells whether the acknowledgment is for plug or unplug event. This needs > to be set accordingly in order the router to send further hot plug > notifications. > > To make it simpler we fill the field unconditionally. Legacy devices do > not look at this field so there should be no problems with them. > > While there rename tb_cfg_error() to tb_cfg_ack_plug() and update the > log message accordingly. The function is only used to ack plug/unplug > events. > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > drivers/thunderbolt/ctl.c | 19 +++++++++++++------ > drivers/thunderbolt/ctl.h | 3 +-- > drivers/thunderbolt/tb.c | 3 +-- > drivers/thunderbolt/tb_msgs.h | 6 +++++- > 4 files changed, 20 insertions(+), 11 deletions(-) > > diff --git a/drivers/thunderbolt/ctl.c b/drivers/thunderbolt/ctl.c > index d97813e80e5f..f77ceae5c7d7 100644 > --- a/drivers/thunderbolt/ctl.c > +++ b/drivers/thunderbolt/ctl.c > @@ -708,19 +708,26 @@ void tb_ctl_stop(struct tb_ctl *ctl) > /* public interface, commands */ > > /** > - * tb_cfg_error() - send error packet > + * tb_cfg_ack_plug() - Ack hot plug/unplug event > + * @ctl: Control channel to use > + * @route: Router that originated the event > + * @port: Port where the hot plug/unplug happened > + * @unplug: Ack hot plug or unplug > * > - * Return: Returns 0 on success or an error code on failure. > + * Call this as response for hot plug/unplug event to ack it. > + * Returns %0 on success or an error code on failure. > */ > -int tb_cfg_error(struct tb_ctl *ctl, u64 route, u32 port, > - enum tb_cfg_error error) > +int tb_cfg_ack_plug(struct tb_ctl *ctl, u64 route, u32 port, bool unplug) > { > struct cfg_error_pkg pkg = { > .header = tb_cfg_make_header(route), > .port = port, > - .error = error, > + .error = TB_CFG_ERROR_ACK_PLUG_EVENT, > + .pg = unplug ? TB_CFG_ERROR_PG_HOT_UNPLUG > + : TB_CFG_ERROR_PG_HOT_PLUG, > }; > - tb_ctl_dbg(ctl, "resetting error on %llx:%x.\n", route, port); > + tb_ctl_dbg(ctl, "acking hot %splug event on %llx:%x\n", > + unplug ? "un" : "", route, port); > return tb_ctl_tx(ctl, &pkg, sizeof(pkg), TB_CFG_PKG_ERROR); > } > > diff --git a/drivers/thunderbolt/ctl.h b/drivers/thunderbolt/ctl.h > index 2f1a1e111110..97cb03b38953 100644 > --- a/drivers/thunderbolt/ctl.h > +++ b/drivers/thunderbolt/ctl.h > @@ -123,8 +123,7 @@ static inline struct tb_cfg_header tb_cfg_make_header(u64 route) > return header; > } > > -int tb_cfg_error(struct tb_ctl *ctl, u64 route, u32 port, > - enum tb_cfg_error error); > +int tb_cfg_ack_plug(struct tb_ctl *ctl, u64 route, u32 port, bool unplug); > struct tb_cfg_result tb_cfg_reset(struct tb_ctl *ctl, u64 route, > int timeout_msec); > struct tb_cfg_result tb_cfg_read_raw(struct tb_ctl *ctl, void *buffer, > diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c > index 54085f67810a..e54d0d89a32d 100644 > --- a/drivers/thunderbolt/tb.c > +++ b/drivers/thunderbolt/tb.c > @@ -768,8 +768,7 @@ static void tb_handle_event(struct tb *tb, enum tb_cfg_pkg_type type, > > route = tb_cfg_get_route(&pkg->header); > > - if (tb_cfg_error(tb->ctl, route, pkg->port, > - TB_CFG_ERROR_ACK_PLUG_EVENT)) { > + if (tb_cfg_ack_plug(tb->ctl, route, pkg->port, pkg->unplug)) { > tb_warn(tb, "could not ack plug event on %llx:%x\n", route, > pkg->port); > } > diff --git a/drivers/thunderbolt/tb_msgs.h b/drivers/thunderbolt/tb_msgs.h > index 3705057723b6..fc208c567953 100644 > --- a/drivers/thunderbolt/tb_msgs.h > +++ b/drivers/thunderbolt/tb_msgs.h > @@ -67,9 +67,13 @@ struct cfg_error_pkg { > u32 zero1:4; > u32 port:6; > u32 zero2:2; /* Both should be zero, still they are different fields. */ > - u32 zero3:16; > + u32 zero3:14; > + u32 pg:2; > } __packed; Meta-comment, how does this work for endian issues? gcc will "always" pack these in the correct way such that they match up to the bits on the wire? thanks, greg k-h
On Tue, Dec 17, 2019 at 03:33:39PM +0300, Mika Westerberg wrote: > USB4 1.0 section 6.4.2.7 specifies a new field (PG) in notification > packet that is sent as response of hot plug/unplug events. This field > tells whether the acknowledgment is for plug or unplug event. This needs > to be set accordingly in order the router to send further hot plug > notifications. > > To make it simpler we fill the field unconditionally. Legacy devices do > not look at this field so there should be no problems with them. > > While there rename tb_cfg_error() to tb_cfg_ack_plug() and update the > log message accordingly. The function is only used to ack plug/unplug > events. > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> First 3 patches look "trivial" enough for me to take right now, any objection to that? Should I be using my usb tree for this? thanks, greg k-h
On Tue, Dec 17, 2019 at 01:46:23PM +0100, Greg Kroah-Hartman wrote: > On Tue, Dec 17, 2019 at 03:33:39PM +0300, Mika Westerberg wrote: > > USB4 1.0 section 6.4.2.7 specifies a new field (PG) in notification > > packet that is sent as response of hot plug/unplug events. This field > > tells whether the acknowledgment is for plug or unplug event. This needs > > to be set accordingly in order the router to send further hot plug > > notifications. > > > > To make it simpler we fill the field unconditionally. Legacy devices do > > not look at this field so there should be no problems with them. > > > > While there rename tb_cfg_error() to tb_cfg_ack_plug() and update the > > log message accordingly. The function is only used to ack plug/unplug > > events. > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > --- > > drivers/thunderbolt/ctl.c | 19 +++++++++++++------ > > drivers/thunderbolt/ctl.h | 3 +-- > > drivers/thunderbolt/tb.c | 3 +-- > > drivers/thunderbolt/tb_msgs.h | 6 +++++- > > 4 files changed, 20 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/thunderbolt/ctl.c b/drivers/thunderbolt/ctl.c > > index d97813e80e5f..f77ceae5c7d7 100644 > > --- a/drivers/thunderbolt/ctl.c > > +++ b/drivers/thunderbolt/ctl.c > > @@ -708,19 +708,26 @@ void tb_ctl_stop(struct tb_ctl *ctl) > > /* public interface, commands */ > > > > /** > > - * tb_cfg_error() - send error packet > > + * tb_cfg_ack_plug() - Ack hot plug/unplug event > > + * @ctl: Control channel to use > > + * @route: Router that originated the event > > + * @port: Port where the hot plug/unplug happened > > + * @unplug: Ack hot plug or unplug > > * > > - * Return: Returns 0 on success or an error code on failure. > > + * Call this as response for hot plug/unplug event to ack it. > > + * Returns %0 on success or an error code on failure. > > */ > > -int tb_cfg_error(struct tb_ctl *ctl, u64 route, u32 port, > > - enum tb_cfg_error error) > > +int tb_cfg_ack_plug(struct tb_ctl *ctl, u64 route, u32 port, bool unplug) > > { > > struct cfg_error_pkg pkg = { > > .header = tb_cfg_make_header(route), > > .port = port, > > - .error = error, > > + .error = TB_CFG_ERROR_ACK_PLUG_EVENT, > > + .pg = unplug ? TB_CFG_ERROR_PG_HOT_UNPLUG > > + : TB_CFG_ERROR_PG_HOT_PLUG, > > }; > > - tb_ctl_dbg(ctl, "resetting error on %llx:%x.\n", route, port); > > + tb_ctl_dbg(ctl, "acking hot %splug event on %llx:%x\n", > > + unplug ? "un" : "", route, port); > > return tb_ctl_tx(ctl, &pkg, sizeof(pkg), TB_CFG_PKG_ERROR); > > } > > > > diff --git a/drivers/thunderbolt/ctl.h b/drivers/thunderbolt/ctl.h > > index 2f1a1e111110..97cb03b38953 100644 > > --- a/drivers/thunderbolt/ctl.h > > +++ b/drivers/thunderbolt/ctl.h > > @@ -123,8 +123,7 @@ static inline struct tb_cfg_header tb_cfg_make_header(u64 route) > > return header; > > } > > > > -int tb_cfg_error(struct tb_ctl *ctl, u64 route, u32 port, > > - enum tb_cfg_error error); > > +int tb_cfg_ack_plug(struct tb_ctl *ctl, u64 route, u32 port, bool unplug); > > struct tb_cfg_result tb_cfg_reset(struct tb_ctl *ctl, u64 route, > > int timeout_msec); > > struct tb_cfg_result tb_cfg_read_raw(struct tb_ctl *ctl, void *buffer, > > diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c > > index 54085f67810a..e54d0d89a32d 100644 > > --- a/drivers/thunderbolt/tb.c > > +++ b/drivers/thunderbolt/tb.c > > @@ -768,8 +768,7 @@ static void tb_handle_event(struct tb *tb, enum tb_cfg_pkg_type type, > > > > route = tb_cfg_get_route(&pkg->header); > > > > - if (tb_cfg_error(tb->ctl, route, pkg->port, > > - TB_CFG_ERROR_ACK_PLUG_EVENT)) { > > + if (tb_cfg_ack_plug(tb->ctl, route, pkg->port, pkg->unplug)) { > > tb_warn(tb, "could not ack plug event on %llx:%x\n", route, > > pkg->port); > > } > > diff --git a/drivers/thunderbolt/tb_msgs.h b/drivers/thunderbolt/tb_msgs.h > > index 3705057723b6..fc208c567953 100644 > > --- a/drivers/thunderbolt/tb_msgs.h > > +++ b/drivers/thunderbolt/tb_msgs.h > > @@ -67,9 +67,13 @@ struct cfg_error_pkg { > > u32 zero1:4; > > u32 port:6; > > u32 zero2:2; /* Both should be zero, still they are different fields. */ > > - u32 zero3:16; > > + u32 zero3:14; > > + u32 pg:2; > > } __packed; > > Meta-comment, how does this work for endian issues? gcc will "always" > pack these in the correct way such that they match up to the bits on the > wire? Good question. I'm not entirely sure. My guess is that this simply does not work properly on a big endian system (judging from what is done in struct iphdr for example). It is on my todo list to eventually get rid of all bit fields that are used to deal with the hardware registers/protocol in this driver. New stuff is not supposed to use bit fields with some exceptions like this one.
On Tue, Dec 17, 2019 at 01:47:45PM +0100, Greg Kroah-Hartman wrote: > On Tue, Dec 17, 2019 at 03:33:39PM +0300, Mika Westerberg wrote: > > USB4 1.0 section 6.4.2.7 specifies a new field (PG) in notification > > packet that is sent as response of hot plug/unplug events. This field > > tells whether the acknowledgment is for plug or unplug event. This needs > > to be set accordingly in order the router to send further hot plug > > notifications. > > > > To make it simpler we fill the field unconditionally. Legacy devices do > > not look at this field so there should be no problems with them. > > > > While there rename tb_cfg_error() to tb_cfg_ack_plug() and update the > > log message accordingly. The function is only used to ack plug/unplug > > events. > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > First 3 patches look "trivial" enough for me to take right now, any > objection to that? No objections from my side :) > Should I be using my usb tree for this? Yes, I think it makes sense now that this is also under USB IF umbrella.
On Tue, Dec 17, 2019 at 04:56:32PM +0200, Mika Westerberg wrote: > On Tue, Dec 17, 2019 at 01:47:45PM +0100, Greg Kroah-Hartman wrote: > > On Tue, Dec 17, 2019 at 03:33:39PM +0300, Mika Westerberg wrote: > > > USB4 1.0 section 6.4.2.7 specifies a new field (PG) in notification > > > packet that is sent as response of hot plug/unplug events. This field > > > tells whether the acknowledgment is for plug or unplug event. This needs > > > to be set accordingly in order the router to send further hot plug > > > notifications. > > > > > > To make it simpler we fill the field unconditionally. Legacy devices do > > > not look at this field so there should be no problems with them. > > > > > > While there rename tb_cfg_error() to tb_cfg_ack_plug() and update the > > > log message accordingly. The function is only used to ack plug/unplug > > > events. > > > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > > > First 3 patches look "trivial" enough for me to take right now, any > > objection to that? > > No objections from my side :) > > > Should I be using my usb tree for this? > > Yes, I think it makes sense now that this is also under USB IF umbrella. Ok, now done, thanks. greg k-h
diff --git a/drivers/thunderbolt/ctl.c b/drivers/thunderbolt/ctl.c index d97813e80e5f..f77ceae5c7d7 100644 --- a/drivers/thunderbolt/ctl.c +++ b/drivers/thunderbolt/ctl.c @@ -708,19 +708,26 @@ void tb_ctl_stop(struct tb_ctl *ctl) /* public interface, commands */ /** - * tb_cfg_error() - send error packet + * tb_cfg_ack_plug() - Ack hot plug/unplug event + * @ctl: Control channel to use + * @route: Router that originated the event + * @port: Port where the hot plug/unplug happened + * @unplug: Ack hot plug or unplug * - * Return: Returns 0 on success or an error code on failure. + * Call this as response for hot plug/unplug event to ack it. + * Returns %0 on success or an error code on failure. */ -int tb_cfg_error(struct tb_ctl *ctl, u64 route, u32 port, - enum tb_cfg_error error) +int tb_cfg_ack_plug(struct tb_ctl *ctl, u64 route, u32 port, bool unplug) { struct cfg_error_pkg pkg = { .header = tb_cfg_make_header(route), .port = port, - .error = error, + .error = TB_CFG_ERROR_ACK_PLUG_EVENT, + .pg = unplug ? TB_CFG_ERROR_PG_HOT_UNPLUG + : TB_CFG_ERROR_PG_HOT_PLUG, }; - tb_ctl_dbg(ctl, "resetting error on %llx:%x.\n", route, port); + tb_ctl_dbg(ctl, "acking hot %splug event on %llx:%x\n", + unplug ? "un" : "", route, port); return tb_ctl_tx(ctl, &pkg, sizeof(pkg), TB_CFG_PKG_ERROR); } diff --git a/drivers/thunderbolt/ctl.h b/drivers/thunderbolt/ctl.h index 2f1a1e111110..97cb03b38953 100644 --- a/drivers/thunderbolt/ctl.h +++ b/drivers/thunderbolt/ctl.h @@ -123,8 +123,7 @@ static inline struct tb_cfg_header tb_cfg_make_header(u64 route) return header; } -int tb_cfg_error(struct tb_ctl *ctl, u64 route, u32 port, - enum tb_cfg_error error); +int tb_cfg_ack_plug(struct tb_ctl *ctl, u64 route, u32 port, bool unplug); struct tb_cfg_result tb_cfg_reset(struct tb_ctl *ctl, u64 route, int timeout_msec); struct tb_cfg_result tb_cfg_read_raw(struct tb_ctl *ctl, void *buffer, diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c index 54085f67810a..e54d0d89a32d 100644 --- a/drivers/thunderbolt/tb.c +++ b/drivers/thunderbolt/tb.c @@ -768,8 +768,7 @@ static void tb_handle_event(struct tb *tb, enum tb_cfg_pkg_type type, route = tb_cfg_get_route(&pkg->header); - if (tb_cfg_error(tb->ctl, route, pkg->port, - TB_CFG_ERROR_ACK_PLUG_EVENT)) { + if (tb_cfg_ack_plug(tb->ctl, route, pkg->port, pkg->unplug)) { tb_warn(tb, "could not ack plug event on %llx:%x\n", route, pkg->port); } diff --git a/drivers/thunderbolt/tb_msgs.h b/drivers/thunderbolt/tb_msgs.h index 3705057723b6..fc208c567953 100644 --- a/drivers/thunderbolt/tb_msgs.h +++ b/drivers/thunderbolt/tb_msgs.h @@ -67,9 +67,13 @@ struct cfg_error_pkg { u32 zero1:4; u32 port:6; u32 zero2:2; /* Both should be zero, still they are different fields. */ - u32 zero3:16; + u32 zero3:14; + u32 pg:2; } __packed; +#define TB_CFG_ERROR_PG_HOT_PLUG 0x2 +#define TB_CFG_ERROR_PG_HOT_UNPLUG 0x3 + /* TB_CFG_PKG_EVENT */ struct cfg_event_pkg { struct tb_cfg_header header;
USB4 1.0 section 6.4.2.7 specifies a new field (PG) in notification packet that is sent as response of hot plug/unplug events. This field tells whether the acknowledgment is for plug or unplug event. This needs to be set accordingly in order the router to send further hot plug notifications. To make it simpler we fill the field unconditionally. Legacy devices do not look at this field so there should be no problems with them. While there rename tb_cfg_error() to tb_cfg_ack_plug() and update the log message accordingly. The function is only used to ack plug/unplug events. Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/thunderbolt/ctl.c | 19 +++++++++++++------ drivers/thunderbolt/ctl.h | 3 +-- drivers/thunderbolt/tb.c | 3 +-- drivers/thunderbolt/tb_msgs.h | 6 +++++- 4 files changed, 20 insertions(+), 11 deletions(-)