[PATCH 34/36] tty: gdm724x: convert counts to size_t

[PATCH 34/36] tty: gdm724x: convert counts to size_t

[Jiri Slaby (SUSE)]

Unify the type of tty_operations::write() counters with the 'count'
parameter. I.e. use size_t for them.

This includes changing constants to UL to keep min() and avoid min_t().

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: linux-staging@lists.linux.dev
---
 drivers/staging/gdm724x/gdm_tty.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/gdm724x/gdm_tty.c b/drivers/staging/gdm724x/gdm_tty.c
index b31f2afb0286..cbaaa8fa7474 100644
--- a/drivers/staging/gdm724x/gdm_tty.c
+++ b/drivers/staging/gdm724x/gdm_tty.c
@@ -17,9 +17,9 @@
 #define GDM_TTY_MAJOR 0
 #define GDM_TTY_MINOR 32
 
-#define WRITE_SIZE 2048
+#define WRITE_SIZE 2048UL
 
-#define MUX_TX_MAX_SIZE 2048
+#define MUX_TX_MAX_SIZE 2048UL
 
 static inline bool gdm_tty_ready(struct gdm *gdm)
 {
@@ -152,9 +152,8 @@ static void gdm_tty_send_complete(void *arg)
 static ssize_t gdm_tty_write(struct tty_struct *tty, const u8 *buf, size_t len)
 {
 	struct gdm *gdm = tty->driver_data;
-	int remain = len;
-	int sent_len = 0;
-	int sending_len = 0;
+	size_t remain = len;
+	size_t sent_len = 0;
 
 	if (!gdm_tty_ready(gdm))
 		return -ENODEV;
@@ -163,7 +162,7 @@ static ssize_t gdm_tty_write(struct tty_struct *tty, const u8 *buf, size_t len)
 		return 0;
 
 	while (1) {
-		sending_len = min(MUX_TX_MAX_SIZE, remain);
+		size_t sending_len = min(MUX_TX_MAX_SIZE, remain);
 		gdm->tty_dev->send_func(gdm->tty_dev->priv_dev,
 					(void *)(buf + sent_len),
 					sending_len,
-- 
2.41.0

Re: [PATCH 34/36] tty: gdm724x: convert counts to size_t

[Dan Carpenter]

On Thu, Aug 10, 2023 at 11:15:08AM +0200, Jiri Slaby (SUSE) wrote:
> Unify the type of tty_operations::write() counters with the 'count'
> parameter. I.e. use size_t for them.
> 
> This includes changing constants to UL to keep min() and avoid min_t().
> 
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Cc: linux-staging@lists.linux.dev
> ---
>  drivers/staging/gdm724x/gdm_tty.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/gdm724x/gdm_tty.c b/drivers/staging/gdm724x/gdm_tty.c
> index b31f2afb0286..cbaaa8fa7474 100644
> --- a/drivers/staging/gdm724x/gdm_tty.c
> +++ b/drivers/staging/gdm724x/gdm_tty.c
> @@ -17,9 +17,9 @@
>  #define GDM_TTY_MAJOR 0
>  #define GDM_TTY_MINOR 32
>  
> -#define WRITE_SIZE 2048
> +#define WRITE_SIZE 2048UL
>  
> -#define MUX_TX_MAX_SIZE 2048
> +#define MUX_TX_MAX_SIZE 2048UL
>  
>  static inline bool gdm_tty_ready(struct gdm *gdm)
>  {
> @@ -152,9 +152,8 @@ static void gdm_tty_send_complete(void *arg)
>  static ssize_t gdm_tty_write(struct tty_struct *tty, const u8 *buf, size_t len)
>  {
>  	struct gdm *gdm = tty->driver_data;
> -	int remain = len;
> -	int sent_len = 0;
> -	int sending_len = 0;
> +	size_t remain = len;

We later check if remain <= 0.  It still works because remain could
never be negative, but now it's even less necessary to check for
negatives, I guess.

regards,
dan carpenter

Re: [PATCH 34/36] tty: gdm724x: convert counts to size_t

[Jiri Slaby]

On 10. 08. 23, 11:42, Dan Carpenter wrote:
> On Thu, Aug 10, 2023 at 11:15:08AM +0200, Jiri Slaby (SUSE) wrote:
>> Unify the type of tty_operations::write() counters with the 'count'
>> parameter. I.e. use size_t for them.
>>
>> This includes changing constants to UL to keep min() and avoid min_t().
>>
>> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
>> Cc: linux-staging@lists.linux.dev
>> ---
>>   drivers/staging/gdm724x/gdm_tty.c | 11 +++++------
>>   1 file changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/gdm724x/gdm_tty.c b/drivers/staging/gdm724x/gdm_tty.c
>> index b31f2afb0286..cbaaa8fa7474 100644
>> --- a/drivers/staging/gdm724x/gdm_tty.c
>> +++ b/drivers/staging/gdm724x/gdm_tty.c
>> @@ -17,9 +17,9 @@
>>   #define GDM_TTY_MAJOR 0
>>   #define GDM_TTY_MINOR 32
>>   
>> -#define WRITE_SIZE 2048
>> +#define WRITE_SIZE 2048UL
>>   
>> -#define MUX_TX_MAX_SIZE 2048
>> +#define MUX_TX_MAX_SIZE 2048UL
>>   
>>   static inline bool gdm_tty_ready(struct gdm *gdm)
>>   {
>> @@ -152,9 +152,8 @@ static void gdm_tty_send_complete(void *arg)
>>   static ssize_t gdm_tty_write(struct tty_struct *tty, const u8 *buf, size_t len)
>>   {
>>   	struct gdm *gdm = tty->driver_data;
>> -	int remain = len;
>> -	int sent_len = 0;
>> -	int sending_len = 0;
>> +	size_t remain = len;
> 
> We later check if remain <= 0.  It still works because remain could
> never be negative, but now it's even less necessary to check for
> negatives, I guess.

You're right. The whole loop should be now:

while (remain) {
   ...
}

Even without the preceding 'if'.

thanks,
-- 
js
suse labs

[PATCH 34-and-three-quarters/36] tty: gdm724x: simplify gdm_tty_write()

[Jiri Slaby (SUSE)]

len and remain can never be negative in gdm_tty_write(). So remove such
a check and move the check of remaining bytes to the loop condition.
This way, the preceding 'if' is now superfluous too. Fix all that and
make the code cleaner.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Cc: linux-staging@lists.linux.dev
---
 drivers/staging/gdm724x/gdm_tty.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/staging/gdm724x/gdm_tty.c b/drivers/staging/gdm724x/gdm_tty.c
index cbaaa8fa7474..67d9bf41e836 100644
--- a/drivers/staging/gdm724x/gdm_tty.c
+++ b/drivers/staging/gdm724x/gdm_tty.c
@@ -158,10 +158,7 @@ static ssize_t gdm_tty_write(struct tty_struct *tty, const u8 *buf, size_t len)
 	if (!gdm_tty_ready(gdm))
 		return -ENODEV;
 
-	if (!len)
-		return 0;
-
-	while (1) {
+	while (remain) {
 		size_t sending_len = min(MUX_TX_MAX_SIZE, remain);
 		gdm->tty_dev->send_func(gdm->tty_dev->priv_dev,
 					(void *)(buf + sent_len),
@@ -171,8 +168,6 @@ static ssize_t gdm_tty_write(struct tty_struct *tty, const u8 *buf, size_t len)
 					gdm);
 		sent_len += sending_len;
 		remain -= sending_len;
-		if (remain <= 0)
-			break;
 	}
 
 	return len;
-- 
2.41.0

Re: [PATCH 34-and-three-quarters/36] tty: gdm724x: simplify gdm_tty_write()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1533 bytes --]

On Thu, 10 Aug 2023, Jiri Slaby (SUSE) wrote:

> len and remain can never be negative in gdm_tty_write(). So remove such
> a check and move the check of remaining bytes to the loop condition.
> This way, the preceding 'if' is now superfluous too. Fix all that and
> make the code cleaner.
> 
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>

I guess Suggested-by would be more appropriate since there's no problem 
being fixed here.

> Cc: linux-staging@lists.linux.dev

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.



> ---
>  drivers/staging/gdm724x/gdm_tty.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/gdm724x/gdm_tty.c b/drivers/staging/gdm724x/gdm_tty.c
> index cbaaa8fa7474..67d9bf41e836 100644
> --- a/drivers/staging/gdm724x/gdm_tty.c
> +++ b/drivers/staging/gdm724x/gdm_tty.c
> @@ -158,10 +158,7 @@ static ssize_t gdm_tty_write(struct tty_struct *tty, const u8 *buf, size_t len)
>  	if (!gdm_tty_ready(gdm))
>  		return -ENODEV;
>  
> -	if (!len)
> -		return 0;
> -
> -	while (1) {
> +	while (remain) {
>  		size_t sending_len = min(MUX_TX_MAX_SIZE, remain);
>  		gdm->tty_dev->send_func(gdm->tty_dev->priv_dev,
>  					(void *)(buf + sent_len),
> @@ -171,8 +168,6 @@ static ssize_t gdm_tty_write(struct tty_struct *tty, const u8 *buf, size_t len)
>  					gdm);
>  		sent_len += sending_len;
>  		remain -= sending_len;
> -		if (remain <= 0)
> -			break;
>  	}
>  
>  	return len;
> 

Re: [PATCH 34/36] tty: gdm724x: convert counts to size_t

[Nathan Chancellor]

On Thu, Aug 10, 2023 at 11:15:08AM +0200, Jiri Slaby (SUSE) wrote:
> Unify the type of tty_operations::write() counters with the 'count'
> parameter. I.e. use size_t for them.
> 
> This includes changing constants to UL to keep min() and avoid min_t().

This patch appears to cause a warning/error on 32-bit architectures now
due to this part of the change, as size_t is 'unsigned int' there:

  In file included from include/linux/kernel.h:27,
                   from drivers/staging/gdm724x/gdm_tty.c:6:
  drivers/staging/gdm724x/gdm_tty.c: In function 'gdm_tty_write':
  include/linux/minmax.h:21:35: error: comparison of distinct pointer types lacks a cast [-Werror]
     21 |         (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
        |                                   ^~
  include/linux/minmax.h:27:18: note: in expansion of macro '__typecheck'
     27 |                 (__typecheck(x, y) && __no_side_effects(x, y))
        |                  ^~~~~~~~~~~
  include/linux/minmax.h:37:31: note: in expansion of macro '__safe_cmp'
     37 |         __builtin_choose_expr(__safe_cmp(x, y), \
        |                               ^~~~~~~~~~
  include/linux/minmax.h:68:25: note: in expansion of macro '__careful_cmp'
     68 | #define min(x, y)       __careful_cmp(x, y, <)
        |                         ^~~~~~~~~~~~~
  drivers/staging/gdm724x/gdm_tty.c:162:38: note: in expansion of macro 'min'
    162 |                 size_t sending_len = min(MUX_TX_MAX_SIZE, remain);
        |                                      ^~~
  cc1: all warnings being treated as errors

> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Cc: linux-staging@lists.linux.dev
> ---
>  drivers/staging/gdm724x/gdm_tty.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/gdm724x/gdm_tty.c b/drivers/staging/gdm724x/gdm_tty.c
> index b31f2afb0286..cbaaa8fa7474 100644
> --- a/drivers/staging/gdm724x/gdm_tty.c
> +++ b/drivers/staging/gdm724x/gdm_tty.c
> @@ -17,9 +17,9 @@
>  #define GDM_TTY_MAJOR 0
>  #define GDM_TTY_MINOR 32
>  
> -#define WRITE_SIZE 2048
> +#define WRITE_SIZE 2048UL
>  
> -#define MUX_TX_MAX_SIZE 2048
> +#define MUX_TX_MAX_SIZE 2048UL
>  
>  static inline bool gdm_tty_ready(struct gdm *gdm)
>  {
> @@ -152,9 +152,8 @@ static void gdm_tty_send_complete(void *arg)
>  static ssize_t gdm_tty_write(struct tty_struct *tty, const u8 *buf, size_t len)
>  {
>  	struct gdm *gdm = tty->driver_data;
> -	int remain = len;
> -	int sent_len = 0;
> -	int sending_len = 0;
> +	size_t remain = len;
> +	size_t sent_len = 0;
>  
>  	if (!gdm_tty_ready(gdm))
>  		return -ENODEV;
> @@ -163,7 +162,7 @@ static ssize_t gdm_tty_write(struct tty_struct *tty, const u8 *buf, size_t len)
>  		return 0;
>  
>  	while (1) {
> -		sending_len = min(MUX_TX_MAX_SIZE, remain);
> +		size_t sending_len = min(MUX_TX_MAX_SIZE, remain);
>  		gdm->tty_dev->send_func(gdm->tty_dev->priv_dev,
>  					(void *)(buf + sent_len),
>  					sending_len,
> -- 
> 2.41.0
> 

Re: [PATCH 34/36] tty: gdm724x: convert counts to size_t

[Jiri Slaby]

On 15. 08. 23, 19:22, Nathan Chancellor wrote:
> On Thu, Aug 10, 2023 at 11:15:08AM +0200, Jiri Slaby (SUSE) wrote:
>> Unify the type of tty_operations::write() counters with the 'count'
>> parameter. I.e. use size_t for them.
>>
>> This includes changing constants to UL to keep min() and avoid min_t().
> 
> This patch appears to cause a warning/error on 32-bit architectures now
> due to this part of the change, as size_t is 'unsigned int' there:

Right, this is my brain fart thinking ulong is the same as size_t 
everywhere. No, size_t is uint on 32bit.

I will fix this -- kernel build bot seems to be slow -- it didn't find 
the issue out in my queue, nor in tty-testing.

thanks,
-- 
js
suse labs

RE: [PATCH 34/36] tty: gdm724x: convert counts to size_t

[David Laight]

From: Jiri Slaby
> Sent: Wednesday, August 16, 2023 7:47 AM
> 
> On 15. 08. 23, 19:22, Nathan Chancellor wrote:
> > On Thu, Aug 10, 2023 at 11:15:08AM +0200, Jiri Slaby (SUSE) wrote:
> >> Unify the type of tty_operations::write() counters with the 'count'
> >> parameter. I.e. use size_t for them.
> >>
> >> This includes changing constants to UL to keep min() and avoid min_t().
> >
> > This patch appears to cause a warning/error on 32-bit architectures now
> > due to this part of the change, as size_t is 'unsigned int' there:
> 
> Right, this is my brain fart thinking ulong is the same as size_t
> everywhere. No, size_t is uint on 32bit.
> 
> I will fix this -- kernel build bot seems to be slow -- it didn't find
> the issue out in my queue, nor in tty-testing.

'Vote up' my patches to minmax.h that make this all work.
Then it won't care provided both values have the same signedness.
(or, with patch 5, are non-negative 31bit compile time constants.)

Pretty much the only other patch is casting the constants to (size_t).

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 34/36] tty: gdm724x: convert counts to size_t

[Jiri Slaby]

On 16. 08. 23, 10:40, David Laight wrote:
> From: Jiri Slaby
>> Sent: Wednesday, August 16, 2023 7:47 AM
>>
>> On 15. 08. 23, 19:22, Nathan Chancellor wrote:
>>> On Thu, Aug 10, 2023 at 11:15:08AM +0200, Jiri Slaby (SUSE) wrote:
>>>> Unify the type of tty_operations::write() counters with the 'count'
>>>> parameter. I.e. use size_t for them.
>>>>
>>>> This includes changing constants to UL to keep min() and avoid min_t().
>>>
>>> This patch appears to cause a warning/error on 32-bit architectures now
>>> due to this part of the change, as size_t is 'unsigned int' there:
>>
>> Right, this is my brain fart thinking ulong is the same as size_t
>> everywhere. No, size_t is uint on 32bit.
>>
>> I will fix this -- kernel build bot seems to be slow -- it didn't find
>> the issue out in my queue, nor in tty-testing.
> 
> 'Vote up' my patches to minmax.h that make this all work.
> Then it won't care provided both values have the same signedness.
> (or, with patch 5, are non-negative 31bit compile time constants.)

Oh yeah, that [1] looks great. Why should one care in min(4096, 
sizeof()) after all…

So what's the current status of those?

[1] 
https://lore.kernel.org/all/b4ce9dad748e489f9314a2dc95615033@AcuMS.aculab.com/

thanks,
-- 
js
suse labs

RE: [PATCH 34/36] tty: gdm724x: convert counts to size_t

[David Laight]

From: Jiri Slaby
> Sent: Wednesday, August 16, 2023 9:59 AM
...
> > 'Vote up' my patches to minmax.h that make this all work.
> > Then it won't care provided both values have the same signedness.
> > (or, with patch 5, are non-negative 31bit compile time constants.)
> 
> Oh yeah, that [1] looks great. Why should one care in min(4096,
> sizeof()) after all…
> 
> So what's the current status of those?

Waiting... :-(

The only comment is from Linus who really doesn't like the idea
that min(signed_var, 4u) should be the same as min(signed_var, 4).
I think he is ok with min(unsigned_var, 4) though.

The min_t(u16,...) I quoted from the console buffer code is
a real bug that was identified by someone else last week.

Really min_t() is just an accident waiting to happen.

	David

> 
> [1]
> https://lore.kernel.org/all/b4ce9dad748e489f9314a2dc95615033@AcuMS.aculab.com/
> 
> thanks,
> --
> js
> suse labs

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)