dri-devel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] lib: string: add function strtolower()
@ 2016-06-30 23:50 Markus Mayer
  2016-06-30 23:50 ` [PATCH 1/6] " Markus Mayer
  2016-06-30 23:50 ` [PATCH 2/6] drm/nouveau/core: make use of new strtolower() function Markus Mayer
  0 siblings, 2 replies; 12+ messages in thread
From: Markus Mayer @ 2016-06-30 23:50 UTC (permalink / raw)
  To: Andrew Morton, Al Viro, Rasmus Villemoes, Chris Metcalf,
	Kees Cook
  Cc: Markus Mayer, dri-devel, nouveau, linux-acpi, devel, speakup,
	devel, linux-scsi, target-devel, linux-kernel

This series introduces a new generic function strtolower(), which
converts strings to lowercase in-place, overwriting the original
string. This kind of functionality is needed in several places in the
kernel. Right now, everybody seems to be implementing their own copy of
this function. So, we replace several custom "strtolower"
implementations with this new library function.

Another driver that also makes use of this function will be submitted
upstream shortly, which prompted this whole exercise.

The changes made here have been compile-tested, but not tried out, due
to lack of required hardware.

This series is based on v4.7-rc5.

Markus Mayer (6):
  lib: string: add function strtolower()
  drm/nouveau/core: make use of new strtolower() function
  ACPICA: make use of new strtolower() function
  ACPI / device_sysfs: make use of new strtolower() function
  staging: speakup: replace spk_strlwr() with strtolower()
  iscsi-target: replace iscsi_initiatorname_tolower() with strtolower()

 drivers/acpi/acpica/utnonansi.c              | 13 +------------
 drivers/acpi/device_sysfs.c                  |  4 +---
 drivers/gpu/drm/nouveau/nvkm/core/firmware.c |  7 +------
 drivers/staging/speakup/kobjects.c           |  2 +-
 drivers/staging/speakup/main.c               |  2 +-
 drivers/staging/speakup/speakup.h            |  1 -
 drivers/staging/speakup/varhandlers.c        | 12 ------------
 drivers/target/iscsi/iscsi_target_nego.c     | 17 +----------------
 include/linux/string.h                       |  1 +
 lib/string.c                                 | 14 ++++++++++++++
 10 files changed, 21 insertions(+), 52 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/6] lib: string: add function strtolower()
  2016-06-30 23:50 [PATCH 0/6] lib: string: add function strtolower() Markus Mayer
@ 2016-06-30 23:50 ` Markus Mayer
  2016-07-01 10:52   ` Jani Nikula
  2016-07-01 21:08   ` Rasmus Villemoes
  2016-06-30 23:50 ` [PATCH 2/6] drm/nouveau/core: make use of new strtolower() function Markus Mayer
  1 sibling, 2 replies; 12+ messages in thread
From: Markus Mayer @ 2016-06-30 23:50 UTC (permalink / raw)
  To: Andrew Morton, Al Viro, Rasmus Villemoes, Chris Metcalf,
	Kees Cook
  Cc: Markus Mayer, dri-devel, nouveau, linux-acpi, devel, speakup,
	devel, linux-scsi, target-devel, linux-kernel

Add a function called strtolower() to convert strings to lower case
in-place, overwriting the original string.

This seems to be a recurring requirement in the kernel that is
currently being solved by several duplicated implementations doing the
same thing.

Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 include/linux/string.h |  1 +
 lib/string.c           | 14 ++++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index 26b6f6a..aad605e 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -116,6 +116,7 @@ extern void * memchr(const void *,int,__kernel_size_t);
 #endif
 void *memchr_inv(const void *s, int c, size_t n);
 char *strreplace(char *s, char old, char new);
+char *strtolower(char *s);
 
 extern void kfree_const(const void *x);
 
diff --git a/lib/string.c b/lib/string.c
index ed83562..6e3b560 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -952,3 +952,17 @@ char *strreplace(char *s, char old, char new)
 	return s;
 }
 EXPORT_SYMBOL(strreplace);
+
+char *strtolower(char *s)
+{
+	char *p;
+
+        if (unlikely(!s))
+                return NULL;
+
+	for (p = s; *p; p++)
+		*p = tolower(*p);
+
+	return s;
+}
+EXPORT_SYMBOL(strtolower);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/6] drm/nouveau/core: make use of new strtolower() function
  2016-06-30 23:50 [PATCH 0/6] lib: string: add function strtolower() Markus Mayer
  2016-06-30 23:50 ` [PATCH 1/6] " Markus Mayer
@ 2016-06-30 23:50 ` Markus Mayer
  2016-07-02  1:18   ` [Nouveau] " Alexandre Courbot
  1 sibling, 1 reply; 12+ messages in thread
From: Markus Mayer @ 2016-06-30 23:50 UTC (permalink / raw)
  To: Ben Skeggs, David Airlie, Alexandre Courbot
  Cc: Markus Mayer, dri-devel, nouveau, linux-kernel

Call strtolower() rather than walking the string explicitly to convert
it to lowercase.

Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 drivers/gpu/drm/nouveau/nvkm/core/firmware.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c
index 34ecd4a..c50594c 100644
--- a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c
+++ b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c
@@ -36,16 +36,11 @@ nvkm_firmware_get(struct nvkm_device *device, const char *fwname,
 {
 	char f[64];
 	char cname[16];
-	int i;
 
 	/* Convert device name to lowercase */
 	strncpy(cname, device->chip->name, sizeof(cname));
 	cname[sizeof(cname) - 1] = '\0';
-	i = strlen(cname);
-	while (i) {
-		--i;
-		cname[i] = tolower(cname[i]);
-	}
+	strtolower(cname);
 
 	snprintf(f, sizeof(f), "nvidia/%s/%s.bin", cname, fwname);
 	return request_firmware(fw, f, device->dev);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/6] lib: string: add function strtolower()
  2016-06-30 23:50 ` [PATCH 1/6] " Markus Mayer
@ 2016-07-01 10:52   ` Jani Nikula
  2016-07-01 17:14     ` Markus Mayer
  2016-07-01 21:08   ` Rasmus Villemoes
  1 sibling, 1 reply; 12+ messages in thread
From: Jani Nikula @ 2016-07-01 10:52 UTC (permalink / raw)
  To: Andrew Morton, Al Viro, Rasmus Villemoes, Chris Metcalf,
	Kees Cook
  Cc: devel, linux-scsi, nouveau, speakup, linux-kernel, dri-devel,
	linux-acpi, target-devel, Markus Mayer, devel

On Fri, 01 Jul 2016, Markus Mayer <mmayer@broadcom.com> wrote:
> Add a function called strtolower() to convert strings to lower case
> in-place, overwriting the original string.
>
> This seems to be a recurring requirement in the kernel that is
> currently being solved by several duplicated implementations doing the
> same thing.
>
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  include/linux/string.h |  1 +
>  lib/string.c           | 14 ++++++++++++++
>  2 files changed, 15 insertions(+)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 26b6f6a..aad605e 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -116,6 +116,7 @@ extern void * memchr(const void *,int,__kernel_size_t);
>  #endif
>  void *memchr_inv(const void *s, int c, size_t n);
>  char *strreplace(char *s, char old, char new);
> +char *strtolower(char *s);
>  
>  extern void kfree_const(const void *x);
>  
> diff --git a/lib/string.c b/lib/string.c
> index ed83562..6e3b560 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -952,3 +952,17 @@ char *strreplace(char *s, char old, char new)
>  	return s;
>  }
>  EXPORT_SYMBOL(strreplace);
> +

This needs a kernel-doc comment right here.

> +char *strtolower(char *s)
> +{
> +	char *p;
> +
> +        if (unlikely(!s))
> +                return NULL;

Using spaces for indentation? See scripts/checkpatch.pl.

> +
> +	for (p = s; *p; p++)
> +		*p = tolower(*p);
> +
> +	return s;

Why does it return a value? Could be void?

BR,
Jani.

> +}
> +EXPORT_SYMBOL(strtolower);

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/6] lib: string: add function strtolower()
  2016-07-01 10:52   ` Jani Nikula
@ 2016-07-01 17:14     ` Markus Mayer
  2016-07-01 17:33       ` Jani Nikula
  0 siblings, 1 reply; 12+ messages in thread
From: Markus Mayer @ 2016-07-01 17:14 UTC (permalink / raw)
  To: Jani Nikula
  Cc: Andrew Morton, Al Viro, Rasmus Villemoes, Chris Metcalf,
	Kees Cook, devel, linux-scsi, nouveau, speakup, Linux Kernel,
	dri-devel, linux-acpi, target-devel, devel

On 1 July 2016 at 03:52, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Fri, 01 Jul 2016, Markus Mayer <mmayer@broadcom.com> wrote:
>> Add a function called strtolower() to convert strings to lower case
>> in-place, overwriting the original string.
>>
>> This seems to be a recurring requirement in the kernel that is
>> currently being solved by several duplicated implementations doing the
>> same thing.
>>
>> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
>> ---
>>  include/linux/string.h |  1 +
>>  lib/string.c           | 14 ++++++++++++++
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/include/linux/string.h b/include/linux/string.h
>> index 26b6f6a..aad605e 100644
>> --- a/include/linux/string.h
>> +++ b/include/linux/string.h
>> @@ -116,6 +116,7 @@ extern void * memchr(const void *,int,__kernel_size_t);
>>  #endif
>>  void *memchr_inv(const void *s, int c, size_t n);
>>  char *strreplace(char *s, char old, char new);
>> +char *strtolower(char *s);
>>
>>  extern void kfree_const(const void *x);
>>
>> diff --git a/lib/string.c b/lib/string.c
>> index ed83562..6e3b560 100644
>> --- a/lib/string.c
>> +++ b/lib/string.c
>> @@ -952,3 +952,17 @@ char *strreplace(char *s, char old, char new)
>>       return s;
>>  }
>>  EXPORT_SYMBOL(strreplace);
>> +
>
> This needs a kernel-doc comment right here.

Will add it.

>> +char *strtolower(char *s)
>> +{
>> +     char *p;
>> +
>> +        if (unlikely(!s))
>> +                return NULL;
>
> Using spaces for indentation? See scripts/checkpatch.pl.

Not on purpose. Thanks for spotting it.

>> +
>> +     for (p = s; *p; p++)
>> +             *p = tolower(*p);
>> +
>> +     return s;
>
> Why does it return a value? Could be void?

It could be void, but I thought that would make the function's use
less flexible. As is, the return value is there if anybody wants it,
but it can be ignored if it is not needed. Also, it seems customary
for string functions to be returning the string that was passed in.

I'll change it to void if there are strong opinions leaning that way.
Personally, I like that it returns a char * better.

> BR,
> Jani.
>
>> +}
>> +EXPORT_SYMBOL(strtolower);
>
> --
> Jani Nikula, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/6] lib: string: add function strtolower()
  2016-07-01 17:14     ` Markus Mayer
@ 2016-07-01 17:33       ` Jani Nikula
  0 siblings, 0 replies; 12+ messages in thread
From: Jani Nikula @ 2016-07-01 17:33 UTC (permalink / raw)
  To: Markus Mayer
  Cc: devel, Kees Cook, linux-scsi, linux-acpi, nouveau,
	Rasmus Villemoes, Linux Kernel, dri-devel, speakup, Chris Metcalf,
	target-devel, Al Viro, Andrew Morton, devel

On Fri, 01 Jul 2016, Markus Mayer <markus.mayer@broadcom.com> wrote:
> On 1 July 2016 at 03:52, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> On Fri, 01 Jul 2016, Markus Mayer <mmayer@broadcom.com> wrote:
>>> Add a function called strtolower() to convert strings to lower case
>>> in-place, overwriting the original string.
>>>
>>> This seems to be a recurring requirement in the kernel that is
>>> currently being solved by several duplicated implementations doing the
>>> same thing.
>>>
>>> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
>>> ---
>>>  include/linux/string.h |  1 +
>>>  lib/string.c           | 14 ++++++++++++++
>>>  2 files changed, 15 insertions(+)
>>>
>>> diff --git a/include/linux/string.h b/include/linux/string.h
>>> index 26b6f6a..aad605e 100644
>>> --- a/include/linux/string.h
>>> +++ b/include/linux/string.h
>>> @@ -116,6 +116,7 @@ extern void * memchr(const void *,int,__kernel_size_t);
>>>  #endif
>>>  void *memchr_inv(const void *s, int c, size_t n);
>>>  char *strreplace(char *s, char old, char new);
>>> +char *strtolower(char *s);
>>>
>>>  extern void kfree_const(const void *x);
>>>
>>> diff --git a/lib/string.c b/lib/string.c
>>> index ed83562..6e3b560 100644
>>> --- a/lib/string.c
>>> +++ b/lib/string.c
>>> @@ -952,3 +952,17 @@ char *strreplace(char *s, char old, char new)
>>>       return s;
>>>  }
>>>  EXPORT_SYMBOL(strreplace);
>>> +
>>
>> This needs a kernel-doc comment right here.
>
> Will add it.
>
>>> +char *strtolower(char *s)
>>> +{
>>> +     char *p;
>>> +
>>> +        if (unlikely(!s))
>>> +                return NULL;
>>
>> Using spaces for indentation? See scripts/checkpatch.pl.
>
> Not on purpose. Thanks for spotting it.
>
>>> +
>>> +     for (p = s; *p; p++)
>>> +             *p = tolower(*p);
>>> +
>>> +     return s;
>>
>> Why does it return a value? Could be void?
>
> It could be void, but I thought that would make the function's use
> less flexible. As is, the return value is there if anybody wants it,
> but it can be ignored if it is not needed. Also, it seems customary
> for string functions to be returning the string that was passed in.
>
> I'll change it to void if there are strong opinions leaning that way.
> Personally, I like that it returns a char * better.

I don't have strong opinions on this. Just a general aversion to
returning something redundant. Avoids questions like, does it allocate a
new string, should I use the return value instead of the string I passed
in, should I check the return value or can I ignore it, should I check
both the string I pass in and the return value for != NULL, etc. But I
could be persuaded either way.

BR,
Jani.


>
>> BR,
>> Jani.
>>
>>> +}
>>> +EXPORT_SYMBOL(strtolower);
>>
>> --
>> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/6] lib: string: add function strtolower()
  2016-06-30 23:50 ` [PATCH 1/6] " Markus Mayer
  2016-07-01 10:52   ` Jani Nikula
@ 2016-07-01 21:08   ` Rasmus Villemoes
  2016-07-04 20:18     ` Markus Mayer
  1 sibling, 1 reply; 12+ messages in thread
From: Rasmus Villemoes @ 2016-07-01 21:08 UTC (permalink / raw)
  To: Markus Mayer
  Cc: Andrew Morton, Al Viro, Chris Metcalf, Kees Cook, dri-devel,
	nouveau, linux-acpi, devel, speakup, devel, linux-scsi,
	target-devel, linux-kernel

On Fri, Jul 01 2016, Markus Mayer <mmayer@broadcom.com> wrote:

> Add a function called strtolower() to convert strings to lower case
> in-place, overwriting the original string.
>
> This seems to be a recurring requirement in the kernel that is
> currently being solved by several duplicated implementations doing the
> same thing.
>
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  include/linux/string.h |  1 +
>  lib/string.c           | 14 ++++++++++++++
>  2 files changed, 15 insertions(+)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 26b6f6a..aad605e 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -116,6 +116,7 @@ extern void * memchr(const void *,int,__kernel_size_t);
>  #endif
>  void *memchr_inv(const void *s, int c, size_t n);
>  char *strreplace(char *s, char old, char new);
> +char *strtolower(char *s);
>  
>  extern void kfree_const(const void *x);
>  
> diff --git a/lib/string.c b/lib/string.c
> index ed83562..6e3b560 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -952,3 +952,17 @@ char *strreplace(char *s, char old, char new)
>  	return s;
>  }
>  EXPORT_SYMBOL(strreplace);
> +
> +char *strtolower(char *s)
> +{
> +	char *p;
> +
> +        if (unlikely(!s))
> +                return NULL;
> +
> +	for (p = s; *p; p++)
> +		*p = tolower(*p);
> +
> +	return s;
> +}
> +EXPORT_SYMBOL(strtolower);

A few suggestions: 

- Make the function take separate src and dst parameters, making it explicitly
  allowed to pass the same value (but not other kinds of overlap, of
  course). That way one can avoid "strcpy(dst, src); strtolower(dst);".

- Drop the NULL check. If someone does "foo->bar = something;
  strtolower(foo->bar); put foo in a global data structure...", the
  dereference of foo->bar may happen much later. Doing the NULL deref
  sooner means it's much easier to find and fix the bug. (Also, other
  str* and mem* functions don't usually check for NULL).

- While it's true that strcpy and memcpy by definition return dst, that's
  mostly useless. If you want it to return anything, please make it
  something that might be used - for example, having stpcpy semantics
  (returning a pointer to dst's terminating \0) means a caller might avoid
  a strlen call.

- Maybe do strtoupper while you're at it. Quick grepping didn't find any
  use for the copy-while-lowercasing, but copy-while-uppercasing can at
  least be used in drivers/acpi/acpica/nsrepair2.c,
  drivers/gpu/drm/nouveau/nvkm/engine/fifo/gk104.c,
  drivers/power/power_supply_sysfs.c along with a bunch of inplace
  uppercasing.


Rasmus

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Nouveau] [PATCH 2/6] drm/nouveau/core: make use of new strtolower() function
  2016-06-30 23:50 ` [PATCH 2/6] drm/nouveau/core: make use of new strtolower() function Markus Mayer
@ 2016-07-02  1:18   ` Alexandre Courbot
  2016-07-02 15:21     ` Markus Mayer
  0 siblings, 1 reply; 12+ messages in thread
From: Alexandre Courbot @ 2016-07-02  1:18 UTC (permalink / raw)
  To: Markus Mayer
  Cc: nouveau@lists.freedesktop.org, Linux Kernel Mailing List,
	dri-devel@lists.freedesktop.org, Ben Skeggs

On Fri, Jul 1, 2016 at 8:50 AM, Markus Mayer <mmayer@broadcom.com> wrote:
> Call strtolower() rather than walking the string explicitly to convert
> it to lowercase.
>
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  drivers/gpu/drm/nouveau/nvkm/core/firmware.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c
> index 34ecd4a..c50594c 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c
> @@ -36,16 +36,11 @@ nvkm_firmware_get(struct nvkm_device *device, const char *fwname,
>  {
>         char f[64];
>         char cname[16];
> -       int i;
>
>         /* Convert device name to lowercase */
>         strncpy(cname, device->chip->name, sizeof(cname));
>         cname[sizeof(cname) - 1] = '\0';
> -       i = strlen(cname);
> -       while (i) {
> -               --i;
> -               cname[i] = tolower(cname[i]);
> -       }
> +       strtolower(cname);

This function doesn't seem to exist as of next-20160701, where have
you found it?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Nouveau] [PATCH 2/6] drm/nouveau/core: make use of new strtolower() function
  2016-07-02  1:18   ` [Nouveau] " Alexandre Courbot
@ 2016-07-02 15:21     ` Markus Mayer
       [not found]       ` <CAGt4E5uusNK3vtBo5N9FyUTuWCpMtQS1KC1mHVCq5iFzFJpJFg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 12+ messages in thread
From: Markus Mayer @ 2016-07-02 15:21 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Ben Skeggs, David Airlie, Alexandre Courbot,
	nouveau@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	Linux Kernel Mailing List

On 1 July 2016 at 18:18, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Fri, Jul 1, 2016 at 8:50 AM, Markus Mayer <mmayer@broadcom.com> wrote:
>> Call strtolower() rather than walking the string explicitly to convert
>> it to lowercase.
>>
>> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
>> ---
>>  drivers/gpu/drm/nouveau/nvkm/core/firmware.c | 7 +------
>>  1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c
>> index 34ecd4a..c50594c 100644
>> --- a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c
>> +++ b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c
>> @@ -36,16 +36,11 @@ nvkm_firmware_get(struct nvkm_device *device, const char *fwname,
>>  {
>>         char f[64];
>>         char cname[16];
>> -       int i;
>>
>>         /* Convert device name to lowercase */
>>         strncpy(cname, device->chip->name, sizeof(cname));
>>         cname[sizeof(cname) - 1] = '\0';
>> -       i = strlen(cname);
>> -       while (i) {
>> -               --i;
>> -               cname[i] = tolower(cname[i]);
>> -       }
>> +       strtolower(cname);
>
> This function doesn't seem to exist as of next-20160701, where have
> you found it?

I didn't find it. I wrote it, because it didn't exist and I needed it.
See: https://lkml.org/lkml/2016/6/30/727 and
https://lkml.org/lkml/2016/6/30/733 (cover letter and first patch in
series).

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/6] drm/nouveau/core: make use of new strtolower() function
       [not found]       ` <CAGt4E5uusNK3vtBo5N9FyUTuWCpMtQS1KC1mHVCq5iFzFJpJFg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2016-07-04  1:37         ` Alexandre Courbot
  2016-07-04  3:39           ` [Nouveau] " Alexandre Courbot
  0 siblings, 1 reply; 12+ messages in thread
From: Alexandre Courbot @ 2016-07-04  1:37 UTC (permalink / raw)
  To: Markus Mayer
  Cc: David Airlie,
	nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
	Linux Kernel Mailing List,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
	Ben Skeggs

On Sun, Jul 3, 2016 at 12:21 AM, Markus Mayer <markus.mayer@broadcom.com> wrote:
> On 1 July 2016 at 18:18, Alexandre Courbot <gnurou@gmail.com> wrote:
>> On Fri, Jul 1, 2016 at 8:50 AM, Markus Mayer <mmayer@broadcom.com> wrote:
>>> Call strtolower() rather than walking the string explicitly to convert
>>> it to lowercase.
>>>
>>> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
>>> ---
>>>  drivers/gpu/drm/nouveau/nvkm/core/firmware.c | 7 +------
>>>  1 file changed, 1 insertion(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c
>>> index 34ecd4a..c50594c 100644
>>> --- a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c
>>> +++ b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c
>>> @@ -36,16 +36,11 @@ nvkm_firmware_get(struct nvkm_device *device, const char *fwname,
>>>  {
>>>         char f[64];
>>>         char cname[16];
>>> -       int i;
>>>
>>>         /* Convert device name to lowercase */
>>>         strncpy(cname, device->chip->name, sizeof(cname));
>>>         cname[sizeof(cname) - 1] = '\0';
>>> -       i = strlen(cname);
>>> -       while (i) {
>>> -               --i;
>>> -               cname[i] = tolower(cname[i]);
>>> -       }
>>> +       strtolower(cname);
>>
>> This function doesn't seem to exist as of next-20160701, where have
>> you found it?
>
> I didn't find it. I wrote it, because it didn't exist and I needed it.
> See: https://lkml.org/lkml/2016/6/30/727 and
> https://lkml.org/lkml/2016/6/30/733 (cover letter and first patch in
> series).

Ah, right - would have been easier to understand if you had sent the
whole series (or at least patches 0 to 2) to us as well. Please do
that for the next version.
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Nouveau] [PATCH 2/6] drm/nouveau/core: make use of new strtolower() function
  2016-07-04  1:37         ` Alexandre Courbot
@ 2016-07-04  3:39           ` Alexandre Courbot
  0 siblings, 0 replies; 12+ messages in thread
From: Alexandre Courbot @ 2016-07-04  3:39 UTC (permalink / raw)
  To: Markus Mayer
  Cc: nouveau@lists.freedesktop.org, Linux Kernel Mailing List,
	dri-devel@lists.freedesktop.org, Ben Skeggs

On Mon, Jul 4, 2016 at 10:37 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Sun, Jul 3, 2016 at 12:21 AM, Markus Mayer <markus.mayer@broadcom.com> wrote:
>> On 1 July 2016 at 18:18, Alexandre Courbot <gnurou@gmail.com> wrote:
>>> On Fri, Jul 1, 2016 at 8:50 AM, Markus Mayer <mmayer@broadcom.com> wrote:
>>>> Call strtolower() rather than walking the string explicitly to convert
>>>> it to lowercase.
>>>>
>>>> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
>>>> ---
>>>>  drivers/gpu/drm/nouveau/nvkm/core/firmware.c | 7 +------
>>>>  1 file changed, 1 insertion(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c
>>>> index 34ecd4a..c50594c 100644
>>>> --- a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c
>>>> +++ b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c
>>>> @@ -36,16 +36,11 @@ nvkm_firmware_get(struct nvkm_device *device, const char *fwname,
>>>>  {
>>>>         char f[64];
>>>>         char cname[16];
>>>> -       int i;
>>>>
>>>>         /* Convert device name to lowercase */
>>>>         strncpy(cname, device->chip->name, sizeof(cname));
>>>>         cname[sizeof(cname) - 1] = '\0';
>>>> -       i = strlen(cname);
>>>> -       while (i) {
>>>> -               --i;
>>>> -               cname[i] = tolower(cname[i]);
>>>> -       }
>>>> +       strtolower(cname);
>>>
>>> This function doesn't seem to exist as of next-20160701, where have
>>> you found it?
>>
>> I didn't find it. I wrote it, because it didn't exist and I needed it.
>> See: https://lkml.org/lkml/2016/6/30/727 and
>> https://lkml.org/lkml/2016/6/30/733 (cover letter and first patch in
>> series).
>
> Ah, right - would have been easier to understand if you had sent the
> whole series (or at least patches 0 to 2) to us as well. Please do
> that for the next version.

... found patches 0 and 1 in my spam folder, for some weird reason.
Apologies for jumping to conclusions.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/6] lib: string: add function strtolower()
  2016-07-01 21:08   ` Rasmus Villemoes
@ 2016-07-04 20:18     ` Markus Mayer
  0 siblings, 0 replies; 12+ messages in thread
From: Markus Mayer @ 2016-07-04 20:18 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: Andrew Morton, Al Viro, Chris Metcalf, Kees Cook, dri-devel,
	nouveau, linux-acpi, devel, speakup, devel, linux-scsi,
	target-devel, Linux Kernel

On 1 July 2016 at 14:08, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> A few suggestions:
>
> - Make the function take separate src and dst parameters, making it explicitly
>   allowed to pass the same value (but not other kinds of overlap, of
>   course). That way one can avoid "strcpy(dst, src); strtolower(dst);".
>
> - Drop the NULL check. If someone does "foo->bar = something;
>   strtolower(foo->bar); put foo in a global data structure...", the
>   dereference of foo->bar may happen much later. Doing the NULL deref
>   sooner means it's much easier to find and fix the bug. (Also, other
>   str* and mem* functions don't usually check for NULL).
>
> - While it's true that strcpy and memcpy by definition return dst, that's
>   mostly useless. If you want it to return anything, please make it
>   something that might be used - for example, having stpcpy semantics
>   (returning a pointer to dst's terminating \0) means a caller might avoid
>   a strlen call.
>
> - Maybe do strtoupper while you're at it. Quick grepping didn't find any
>   use for the copy-while-lowercasing, but copy-while-uppercasing can at
>   least be used in drivers/acpi/acpica/nsrepair2.c,
>   drivers/gpu/drm/nouveau/nvkm/engine/fifo/gk104.c,
>   drivers/power/power_supply_sysfs.c along with a bunch of inplace
>   uppercasing.
>
>
> Rasmus

Thanks for the suggestions to you and Jani. Based on the feedback I
received, I am reworking the series now and will post v2 probably
tomorrow.

Regards,
-Markus

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2016-07-04 20:18 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-06-30 23:50 [PATCH 0/6] lib: string: add function strtolower() Markus Mayer
2016-06-30 23:50 ` [PATCH 1/6] " Markus Mayer
2016-07-01 10:52   ` Jani Nikula
2016-07-01 17:14     ` Markus Mayer
2016-07-01 17:33       ` Jani Nikula
2016-07-01 21:08   ` Rasmus Villemoes
2016-07-04 20:18     ` Markus Mayer
2016-06-30 23:50 ` [PATCH 2/6] drm/nouveau/core: make use of new strtolower() function Markus Mayer
2016-07-02  1:18   ` [Nouveau] " Alexandre Courbot
2016-07-02 15:21     ` Markus Mayer
     [not found]       ` <CAGt4E5uusNK3vtBo5N9FyUTuWCpMtQS1KC1mHVCq5iFzFJpJFg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-07-04  1:37         ` Alexandre Courbot
2016-07-04  3:39           ` [Nouveau] " Alexandre Courbot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox