diff mbox series

[NET] net: atlantic: Use readx_poll_timeout() for large timeout

Message ID 20200818161439.3dkf6jzp3vuwmvvh@linutronix.de
State Accepted
Delegated to: David Miller
Headers show
Series [NET] net: atlantic: Use readx_poll_timeout() for large timeout | expand

Commit Message

Sebastian Andrzej Siewior Aug. 18, 2020, 4:14 p.m. UTC
Commit
   8dcf2ad39fdb2 ("net: atlantic: add hwmon getter for MAC temperature")

implemented a read callback with an udelay(10000U). This fails to
compile on ARM because the delay is >1ms. I doubt that it is needed to
spin for 10ms even if possible on x86.

From looking at the code, the context appears to be preemptible so using
usleep() should work and avoid busy spinning.

Use readx_poll_timeout() in the poll loop.

Cc: Mark Starovoytov <mstarovoitov@marvell.com>
Cc: Igor Russkikh <irusskikh@marvell.com>
Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---

Could someone with hardware please verify it? It compiles, yes.

 drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Guenter Roeck Aug. 19, 2020, 4:19 p.m. UTC | #1
On Tue, Aug 18, 2020 at 06:14:39PM +0200, Sebastian Andrzej Siewior wrote:
> Commit
>    8dcf2ad39fdb2 ("net: atlantic: add hwmon getter for MAC temperature")
> 
> implemented a read callback with an udelay(10000U). This fails to
> compile on ARM because the delay is >1ms. I doubt that it is needed to
> spin for 10ms even if possible on x86.
> 
> >From looking at the code, the context appears to be preemptible so using
> usleep() should work and avoid busy spinning.
> 
> Use readx_poll_timeout() in the poll loop.
> 
> Cc: Mark Starovoytov <mstarovoitov@marvell.com>
> Cc: Igor Russkikh <irusskikh@marvell.com>
> Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>

Fixes: 8dcf2ad39fdb2 ("net: atlantic: add hwmon getter for MAC temperature")
Acked-by: Guenter Roeck <linux@roeck-us.net>

As in: This patch does not cause any additional trouble and will fix the
observed compile failure. However, the submitter of 8dcf2ad39fdb2 might
consider adding a mutex either into hw_atl_b0_get_mac_temp() or into
the calling code.

Thanks,
Guenter

> ---
> 
> Could someone with hardware please verify it? It compiles, yes.
> 
>  drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
> index 16a944707ba90..8941ac4df9e37 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
> @@ -1631,8 +1631,8 @@ static int hw_atl_b0_get_mac_temp(struct aq_hw_s *self, u32 *temp)
>  		hw_atl_ts_reset_set(self, 0);
>  	}
>  
> -	err = readx_poll_timeout_atomic(hw_atl_b0_ts_ready_and_latch_high_get,
> -					self, val, val == 1, 10000U, 500000U);
> +	err = readx_poll_timeout(hw_atl_b0_ts_ready_and_latch_high_get, self,
> +				 val, val == 1, 10000U, 500000U);
>  	if (err)
>  		return err;
>  
> -- 
> 2.28.0
>
David Miller Aug. 19, 2020, 11:25 p.m. UTC | #2
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Date: Tue, 18 Aug 2020 18:14:39 +0200

> Commit
>    8dcf2ad39fdb2 ("net: atlantic: add hwmon getter for MAC temperature")
> 
> implemented a read callback with an udelay(10000U). This fails to
> compile on ARM because the delay is >1ms. I doubt that it is needed to
> spin for 10ms even if possible on x86.
> 
> From looking at the code, the context appears to be preemptible so using
> usleep() should work and avoid busy spinning.
> 
> Use readx_poll_timeout() in the poll loop.
> 
> Cc: Mark Starovoytov <mstarovoitov@marvell.com>
> Cc: Igor Russkikh <irusskikh@marvell.com>
> Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>

Applied, thanks.
Igor Russkikh Aug. 20, 2020, 10:17 a.m. UTC | #3
>> implemented a read callback with an udelay(10000U). This fails to
>> compile on ARM because the delay is >1ms. I doubt that it is needed to
>> spin for 10ms even if possible on x86.
>>
>> >From looking at the code, the context appears to be preemptible so
> using
>> usleep() should work and avoid busy spinning.
>>
>> Use readx_poll_timeout() in the poll loop.
>>
>> Cc: Mark Starovoytov <mstarovoitov@marvell.com>
>> Cc: Igor Russkikh <irusskikh@marvell.com>
>> Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
> 
> Fixes: 8dcf2ad39fdb2 ("net: atlantic: add hwmon getter for MAC
> temperature")
> Acked-by: Guenter Roeck <linux@roeck-us.net>
> 
> As in: This patch does not cause any additional trouble and will fix the
> observed compile failure. However, the submitter of 8dcf2ad39fdb2 might
> consider adding a mutex either into hw_atl_b0_get_mac_temp() or into
> the calling code.

Hi Sebastian, Guenter, thanks for catching and taking care of this,
Looks good for me so far.

>> Could someone with hardware please verify it? It compiles, yes.
>>

We'll verify this on our side, sure.

Regards,
  Igor
diff mbox series

Patch

diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
index 16a944707ba90..8941ac4df9e37 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
@@ -1631,8 +1631,8 @@  static int hw_atl_b0_get_mac_temp(struct aq_hw_s *self, u32 *temp)
 		hw_atl_ts_reset_set(self, 0);
 	}
 
-	err = readx_poll_timeout_atomic(hw_atl_b0_ts_ready_and_latch_high_get,
-					self, val, val == 1, 10000U, 500000U);
+	err = readx_poll_timeout(hw_atl_b0_ts_ready_and_latch_high_get, self,
+				 val, val == 1, 10000U, 500000U);
 	if (err)
 		return err;