[Intel-wired-lan] [PATCH v3 net] ice: Write all GNSS buffers instead of first one

[Intel-wired-lan] [PATCH v3 net] ice: Write all GNSS buffers instead of first one

[Karol Kolacinski]

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(-)

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

Re: [Intel-wired-lan] [PATCH v3 net] ice: Write all GNSS buffers instead of first one

[Michal Schmidt]

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
>

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH v3 net] ice: Write all GNSS buffers instead of first one

[Mekala, SunithaX D]

>-----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)
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan