| Message ID | 20200724174702.61754-6-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 754498c1d6369d47b6433e9a05ba926255c2d699 |
| Headers | show |
| Series | [v2,1/6] usb: typec: tcpm: Move mod_delayed_work(&port->vdm_state_machine) call into tcpm_queue_vdm() | expand |
Hello. On 7/24/20 8:47 PM, Hans de Goede wrote: > The tcpm.c code for sending VDMs assumes that there will only be one VDM > in flight at the time. The "queue" used by tcpm_queue_vdm is only 1 entry > deep. > > This assumes that the higher layers (tcpm state-machine and alt-mode > drivers) ensure that queuing a new VDM before the old one has been > completely send (or it timed out) add a WARN_ON to check for this. Sent? > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v2: > - Fix typo in commit-msg subject Another typo? :-) [...] MBR, Sergei
Hi, On 7/24/20 7:57 PM, Sergei Shtylyov wrote: > Hello. > > On 7/24/20 8:47 PM, Hans de Goede wrote: > >> The tcpm.c code for sending VDMs assumes that there will only be one VDM >> in flight at the time. The "queue" used by tcpm_queue_vdm is only 1 entry >> deep. >> >> This assumes that the higher layers (tcpm state-machine and alt-mode >> drivers) ensure that queuing a new VDM before the old one has been >> completely send (or it timed out) add a WARN_ON to check for this. > > Sent? The dictionary says "has been sending" and gives no "has been sen[d|t]" answer. I guess you might be right. Anyways not worth respinning the series for IMHO. Regards, Hans > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Changes in v2: >> - Fix typo in commit-msg subject > > Another typo? :-) > > [...] > > MBR, Sergei >
On 7/24/20 10:47 AM, Hans de Goede wrote: > The tcpm.c code for sending VDMs assumes that there will only be one VDM > in flight at the time. The "queue" used by tcpm_queue_vdm is only 1 entry > deep. > > This assumes that the higher layers (tcpm state-machine and alt-mode > drivers) ensure that queuing a new VDM before the old one has been > completely send (or it timed out) add a WARN_ON to check for this. Just in case you resend ... "has been sent" is grammatically correct. The feedback on https://www.quora.com/What-is-the-difference-between-send-and-sent explains it in detail. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net> Thanks, Guenter > --- > Changes in v2: > - Fix typo in commit-msg subject > --- > drivers/usb/typec/tcpm/tcpm.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index 9b26b57a0172..cc786d558f14 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -971,6 +971,9 @@ static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header, > { > WARN_ON(!mutex_is_locked(&port->lock)); > > + /* Make sure we are not still processing a previous VDM packet */ > + WARN_ON(port->vdm_state > VDM_STATE_DONE); > + > port->vdo_count = cnt + 1; > port->vdo_data[0] = header; > memcpy(&port->vdo_data[1], data, sizeof(u32) * cnt); >
On Fri, Jul 24, 2020 at 07:47:02PM +0200, Hans de Goede wrote: > The tcpm.c code for sending VDMs assumes that there will only be one VDM > in flight at the time. The "queue" used by tcpm_queue_vdm is only 1 entry > deep. > > This assumes that the higher layers (tcpm state-machine and alt-mode > drivers) ensure that queuing a new VDM before the old one has been > completely send (or it timed out) add a WARN_ON to check for this. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > Changes in v2: > - Fix typo in commit-msg subject > --- > drivers/usb/typec/tcpm/tcpm.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index 9b26b57a0172..cc786d558f14 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -971,6 +971,9 @@ static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header, > { > WARN_ON(!mutex_is_locked(&port->lock)); > > + /* Make sure we are not still processing a previous VDM packet */ > + WARN_ON(port->vdm_state > VDM_STATE_DONE); > + > port->vdo_count = cnt + 1; > port->vdo_data[0] = header; > memcpy(&port->vdo_data[1], data, sizeof(u32) * cnt); > -- > 2.26.2 thanks,
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index 9b26b57a0172..cc786d558f14 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -971,6 +971,9 @@ static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header, { WARN_ON(!mutex_is_locked(&port->lock)); + /* Make sure we are not still processing a previous VDM packet */ + WARN_ON(port->vdm_state > VDM_STATE_DONE); + port->vdo_count = cnt + 1; port->vdo_data[0] = header; memcpy(&port->vdo_data[1], data, sizeof(u32) * cnt);
The tcpm.c code for sending VDMs assumes that there will only be one VDM in flight at the time. The "queue" used by tcpm_queue_vdm is only 1 entry deep. This assumes that the higher layers (tcpm state-machine and alt-mode drivers) ensure that queuing a new VDM before the old one has been completely send (or it timed out) add a WARN_ON to check for this. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v2: - Fix typo in commit-msg subject --- drivers/usb/typec/tcpm/tcpm.c | 3 +++ 1 file changed, 3 insertions(+)