| Message ID | 20230217120541.16745-1-karol.kolacinski@intel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Anthony Nguyen |
| Headers | show |
| Series | [v3,net] ice: Write all GNSS buffers instead of first one | expand |
On Fri, Feb 17, 2023 at 1:06 PM Karol Kolacinski <karol.kolacinski@intel.com> wrote: > > When user writes multiple messages in a short period of time, the driver > writes only the first one and buffers others in a linked list. > > Fix this behavior to write all pending buffers instead of only the first > one. > > To reproduce this issue, open the GNSS device with cat, send a few > messages to the device, e.g. multiple commands using echo. The issue > manifests itself as response to only first message. Then, after issuing > a single or multiple commands, user can see that response from the > device was not for recent ones but for the next single buffered one from > the first batch. Although the patch does fix the described issue in my testing, I believe the buffering must be eliminated. See my patch "ice: make writes to /dev/gnssX synchronous", https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20230324162056.200752-1-mschmidt@redhat.com/ Michal > Fixes: d6b98c8d242a ("ice: add write functionality for GNSS TTY") > Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> > --- > V2 -> V3: Switched to net-queue tree instead of next-queue. > V1 -> V2: Added reproduction steps in the commit message. > > drivers/net/ethernet/intel/ice/ice_gnss.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c > index 43e199b5b513..02533014f24a 100644 > --- a/drivers/net/ethernet/intel/ice/ice_gnss.c > +++ b/drivers/net/ethernet/intel/ice/ice_gnss.c > @@ -82,7 +82,7 @@ static void ice_gnss_write_pending(struct kthread_work *work) > write_work); > struct ice_pf *pf = gnss->back; > > - if (!list_empty(&gnss->queue)) { > + while (!list_empty(&gnss->queue)) { > struct gnss_write_buf *write_buf = NULL; > unsigned int bytes; > > -- > 2.37.2 > > _______________________________________________ > Intel-wired-lan mailing list > Intel-wired-lan@osuosl.org > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan >
>-----Original Message----- > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Michal Schmidt > Sent: Friday, March 24, 2023 9:48 AM > To: Kolacinski, Karol <karol.kolacinski@intel.com> > Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; intel-wired-lan@lists.osuosl.org > Subject: Re: [Intel-wired-lan] [PATCH v3 net] ice: Write all GNSS buffers instead of first one > > On Fri, Feb 17, 2023 at 1:06 PM Karol Kolacinski <karol.kolacinski@intel.com> wrote: >> >> When user writes multiple messages in a short period of time, the >> driver writes only the first one and buffers others in a linked list. >> >> Fix this behavior to write all pending buffers instead of only the >> first one. >> >> To reproduce this issue, open the GNSS device with cat, send a few >> messages to the device, e.g. multiple commands using echo. The issue >> manifests itself as response to only first message. Then, after >> issuing a single or multiple commands, user can see that response from >> the device was not for recent ones but for the next single buffered >> one from the first batch. > >Although the patch does fix the described issue in my testing, I believe the buffering must be eliminated. >See my patch "ice: make writes to /dev/gnssX synchronous", https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20230324162056.200752-1-mschmidt@redhat.com/ > >Michal > >> Fixes: d6b98c8d242a ("ice: add write functionality for GNSS TTY") >>Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> >> --- >> V2 -> V3: Switched to net-queue tree instead of next-queue. >> V1 -> V2: Added reproduction steps in the commit message. >> >> drivers/net/ethernet/intel/ice/ice_gnss.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > Tested-by: Sunitha Mekala <sunithax.d.mekala@intel.com> (A Contingent worker at Intel)
diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c index 43e199b5b513..02533014f24a 100644 --- a/drivers/net/ethernet/intel/ice/ice_gnss.c +++ b/drivers/net/ethernet/intel/ice/ice_gnss.c @@ -82,7 +82,7 @@ static void ice_gnss_write_pending(struct kthread_work *work) write_work); struct ice_pf *pf = gnss->back; - if (!list_empty(&gnss->queue)) { + while (!list_empty(&gnss->queue)) { struct gnss_write_buf *write_buf = NULL; unsigned int bytes;
When user writes multiple messages in a short period of time, the driver writes only the first one and buffers others in a linked list. Fix this behavior to write all pending buffers instead of only the first one. To reproduce this issue, open the GNSS device with cat, send a few messages to the device, e.g. multiple commands using echo. The issue manifests itself as response to only first message. Then, after issuing a single or multiple commands, user can see that response from the device was not for recent ones but for the next single buffered one from the first batch. Fixes: d6b98c8d242a ("ice: add write functionality for GNSS TTY") Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> --- V2 -> V3: Switched to net-queue tree instead of next-queue. V1 -> V2: Added reproduction steps in the commit message. drivers/net/ethernet/intel/ice/ice_gnss.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)