Re: [PATCH] USB: serial: ftdi_sio: add IDs for Xsens Mti USB converter

[Frans Klaver]

Hi Patrick,

On Wed, Aug 5, 2020 at 9:57 PM Patrick Riphagen  wrote:
>
> The device added has an FTDI chip inside.
> The device is used to connect Xsens USB Motion Trackers.
>
> Signed-off-by: Patrick Riphagen 

Now you've dropped the backport to stable. Just put

Cc: sta...@vger.kernel.org

just before your sign-off. Everything will be taken care of
automatically once the patch is merged.

Frans

Re: [PATCH v2] [media] lirc_zilog: Clean up lirc zilog error codes

[Frans Klaver]

Almost there on the subject. Stuff between brackets is removed by git,
so you should rather use something like

[PATCH v2] staging: lirc: Clean up zilog error codes

On Wed, Jul 12, 2017 at 9:17 PM, Yves Lemée  wrote:
> According the coding style guidelines, the ENOSYS error code must be returned
> in case of a non existent system call. This code has been replaced with
> the ENOTTY error code indicating a missing functionality.
>
> v2: Improved punctuation
> Fixed patch subject

This change info goes below the --- line and just above the diffstat.


> Signed-off-by: Yves Lemée 
> ---
>  drivers/staging/media/lirc/lirc_zilog.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/media/lirc/lirc_zilog.c 
> b/drivers/staging/media/lirc/lirc_zilog.c
> index 015e41bd036e..26dd32d5b5b2 100644
> --- a/drivers/staging/media/lirc/lirc_zilog.c
> +++ b/drivers/staging/media/lirc/lirc_zilog.c
> @@ -1249,7 +1249,7 @@ static long ioctl(struct file *filep, unsigned int cmd, 
> unsigned long arg)
> break;
> case LIRC_GET_REC_MODE:
> if (!(features & LIRC_CAN_REC_MASK))
> -   return -ENOSYS;
> +   return -ENOTTY;
>
> result = put_user(LIRC_REC2MODE
>   (features & LIRC_CAN_REC_MASK),
> @@ -1257,21 +1257,21 @@ static long ioctl(struct file *filep, unsigned int 
> cmd, unsigned long arg)
> break;
> case LIRC_SET_REC_MODE:
> if (!(features & LIRC_CAN_REC_MASK))
> -   return -ENOSYS;
> +   return -ENOTTY;
>
> result = get_user(mode, uptr);
> if (!result && !(LIRC_MODE2REC(mode) & features))
> -   result = -EINVAL;
> +   result = -ENOTTY;
> break;
> case LIRC_GET_SEND_MODE:
> if (!(features & LIRC_CAN_SEND_MASK))
> -   return -ENOSYS;
> +   return -ENOTTY;
>
> result = put_user(LIRC_MODE_PULSE, uptr);
> break;
> case LIRC_SET_SEND_MODE:
> if (!(features & LIRC_CAN_SEND_MASK))
> -   return -ENOSYS;
> +   return -ENOTTY;
>
> result = get_user(mode, uptr);
> if (!result && mode != LIRC_MODE_PULSE)
> --
> 2.13.2
>
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] [media] lirc_zilog: Clean up lirc zilog error codes

[Frans Klaver]

Almost there on the subject. Stuff between brackets is removed by git,
so you should rather use something like

[PATCH v2] staging: lirc: Clean up zilog error codes

On Wed, Jul 12, 2017 at 9:17 PM, Yves Lemée  wrote:
> According the coding style guidelines, the ENOSYS error code must be returned
> in case of a non existent system call. This code has been replaced with
> the ENOTTY error code indicating a missing functionality.
>
> v2: Improved punctuation
> Fixed patch subject

This change info goes below the --- line and just above the diffstat.


> Signed-off-by: Yves Lemée 
> ---
>  drivers/staging/media/lirc/lirc_zilog.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/media/lirc/lirc_zilog.c 
> b/drivers/staging/media/lirc/lirc_zilog.c
> index 015e41bd036e..26dd32d5b5b2 100644
> --- a/drivers/staging/media/lirc/lirc_zilog.c
> +++ b/drivers/staging/media/lirc/lirc_zilog.c
> @@ -1249,7 +1249,7 @@ static long ioctl(struct file *filep, unsigned int cmd, 
> unsigned long arg)
> break;
> case LIRC_GET_REC_MODE:
> if (!(features & LIRC_CAN_REC_MASK))
> -   return -ENOSYS;
> +   return -ENOTTY;
>
> result = put_user(LIRC_REC2MODE
>   (features & LIRC_CAN_REC_MASK),
> @@ -1257,21 +1257,21 @@ static long ioctl(struct file *filep, unsigned int 
> cmd, unsigned long arg)
> break;
> case LIRC_SET_REC_MODE:
> if (!(features & LIRC_CAN_REC_MASK))
> -   return -ENOSYS;
> +   return -ENOTTY;
>
> result = get_user(mode, uptr);
> if (!result && !(LIRC_MODE2REC(mode) & features))
> -   result = -EINVAL;
> +   result = -ENOTTY;
> break;
> case LIRC_GET_SEND_MODE:
> if (!(features & LIRC_CAN_SEND_MASK))
> -   return -ENOSYS;
> +   return -ENOTTY;
>
> result = put_user(LIRC_MODE_PULSE, uptr);
> break;
> case LIRC_SET_SEND_MODE:
> if (!(features & LIRC_CAN_SEND_MASK))
> -   return -ENOSYS;
> +   return -ENOTTY;
>
> result = get_user(mode, uptr);
> if (!result && mode != LIRC_MODE_PULSE)
> --
> 2.13.2
>
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: ks7010: fix styling WARNINGs

[Frans Klaver]

On Fri, Jun 30, 2017 at 8:39 PM, Mark Rogers  wrote:
> Thank you for your feedback. I guess when making this patch I had the
> preferred coding style in mind, but didn't ask myself if making the code
> conform to it would truly improve readability.
>
> I agree with all of your comments. Do you think the best course of
> action is to create a new patch with this change alone and forget the
> rest?

It's up to you. You could send a couple of patches fixing these
things, or just do one patch fixing one thing.

>
> -   DPRINTK(1, "ks7010_sdio_remove()\n");
> +   DPRINTK(1, "%s()\n", __func__);
>
> Sorry about the newbie questions and bad patch, I will do better with
> the next one. Thanks again for your time and feedback, I really
> appreciate it.

It's pretty much what staging is for. Don't worry about it.

Frans

Re: [PATCH] staging: ks7010: fix styling WARNINGs

[Frans Klaver]

On Fri, Jun 30, 2017 at 8:39 PM, Mark Rogers  wrote:
> Thank you for your feedback. I guess when making this patch I had the
> preferred coding style in mind, but didn't ask myself if making the code
> conform to it would truly improve readability.
>
> I agree with all of your comments. Do you think the best course of
> action is to create a new patch with this change alone and forget the
> rest?

It's up to you. You could send a couple of patches fixing these
things, or just do one patch fixing one thing.

>
> -   DPRINTK(1, "ks7010_sdio_remove()\n");
> +   DPRINTK(1, "%s()\n", __func__);
>
> Sorry about the newbie questions and bad patch, I will do better with
> the next one. Thanks again for your time and feedback, I really
> appreciate it.

It's pretty much what staging is for. Don't worry about it.

Frans

Re: [PATCH] Clean up lirc zilog error codes

[Frans Klaver]

On Tue, Jul 11, 2017 at 7:57 PM, Yves Lemée  wrote:
> According the coding style guidelines, the ENOSYS error code must be returned
> in case of a non existent system call. This code has been replaced with
> the ENOTTY error code indicating, a missing functionality.
>
> Signed-off-by: Yves Lemée 

Your subject line is not according to the patch submission guidelines.

Also, on a nit-picking note, that comma after 'indicating' is rather
oddly placed. Please move or remove it.

Frans

Re: [PATCH] Clean up lirc zilog error codes

[Frans Klaver]

On Tue, Jul 11, 2017 at 7:57 PM, Yves Lemée  wrote:
> According the coding style guidelines, the ENOSYS error code must be returned
> in case of a non existent system call. This code has been replaced with
> the ENOTTY error code indicating, a missing functionality.
>
> Signed-off-by: Yves Lemée 

Your subject line is not according to the patch submission guidelines.

Also, on a nit-picking note, that comma after 'indicating' is rather
oddly placed. Please move or remove it.

Frans

Re: [PATCH v2 2/2] Staging: android/ion: fix sparse warning

[Frans Klaver]

Hi,

Again, your subject is too generic.


On Wed, Jul 12, 2017 at 6:51 AM, Joseph Wright  wrote:
> Declare private function static to fix sparse warning:
>
> ion_cma_heap.c:109:5: warning: symbol '__ion_add_cma_heaps' \
> was not declared. Should it be static?
>
> Signed-off-by: Joseph Wright 
> ---
> Changes in v2:
>   - Split into multiple patches
>
>  drivers/staging/android/ion/ion_cma_heap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/android/ion/ion_cma_heap.c 
> b/drivers/staging/android/ion/ion_cma_heap.c
> index a0949bc..c6db9b7 100644
> --- a/drivers/staging/android/ion/ion_cma_heap.c
> +++ b/drivers/staging/android/ion/ion_cma_heap.c
> @@ -106,7 +106,7 @@ static struct ion_heap *__ion_cma_heap_create(struct cma 
> *cma)
> return _heap->heap;
>  }
>
> -int __ion_add_cma_heaps(struct cma *cma, void *data)
> +static int __ion_add_cma_heaps(struct cma *cma, void *data)
>  {
> struct ion_heap *heap;
>
> --
> 2.9.3
>
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 2/2] Staging: android/ion: fix sparse warning

[Frans Klaver]

Hi,

Again, your subject is too generic.


On Wed, Jul 12, 2017 at 6:51 AM, Joseph Wright  wrote:
> Declare private function static to fix sparse warning:
>
> ion_cma_heap.c:109:5: warning: symbol '__ion_add_cma_heaps' \
> was not declared. Should it be static?
>
> Signed-off-by: Joseph Wright 
> ---
> Changes in v2:
>   - Split into multiple patches
>
>  drivers/staging/android/ion/ion_cma_heap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/android/ion/ion_cma_heap.c 
> b/drivers/staging/android/ion/ion_cma_heap.c
> index a0949bc..c6db9b7 100644
> --- a/drivers/staging/android/ion/ion_cma_heap.c
> +++ b/drivers/staging/android/ion/ion_cma_heap.c
> @@ -106,7 +106,7 @@ static struct ion_heap *__ion_cma_heap_create(struct cma 
> *cma)
> return _heap->heap;
>  }
>
> -int __ion_add_cma_heaps(struct cma *cma, void *data)
> +static int __ion_add_cma_heaps(struct cma *cma, void *data)
>  {
> struct ion_heap *heap;
>
> --
> 2.9.3
>
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/2] Staging: android/ion: fix sparse warnings

[Frans Klaver]

Hi,

please consider changing your subject to something like

staging: android/ion: declare two functions

Perhaps you can make it more on-topic. It's more useful than "fix
sparse warning"

On Wed, Jul 12, 2017 at 6:51 AM, Joseph Wright  wrote:
> Declare functions to fix sparse warnings:
>
> ion_carveout_heap.c:115:17: warning: symbol 'ion_carveout_heap_create' \
> was not declared. Should it be static?
> ion_chunk_heap.c:120:17: warning: symbol 'ion_chunk_heap_create' \
> was not declared. Should it be static?

And then explain why declaring them is preferred over making them static.

Frans

Re: [PATCH v2 1/2] Staging: android/ion: fix sparse warnings

[Frans Klaver]

Hi,

please consider changing your subject to something like

staging: android/ion: declare two functions

Perhaps you can make it more on-topic. It's more useful than "fix
sparse warning"

On Wed, Jul 12, 2017 at 6:51 AM, Joseph Wright  wrote:
> Declare functions to fix sparse warnings:
>
> ion_carveout_heap.c:115:17: warning: symbol 'ion_carveout_heap_create' \
> was not declared. Should it be static?
> ion_chunk_heap.c:120:17: warning: symbol 'ion_chunk_heap_create' \
> was not declared. Should it be static?

And then explain why declaring them is preferred over making them static.

Frans

Re: [PATCH] Staging:ks7010:ks_wlan_net.c: unneeded type casting removed

[Frans Klaver]

On Mon, Jul 10, 2017 at 9:09 AM, AndyS  wrote:
> Subject: [PATCH] Staging:ks7010:ks_wlan_net.c: unneeded type casting removed

[PATCH] staging: ks7010: remove unneeded type casting

> removed undesired type casting. Warning was raised by checkpatch.pl
> This patch is for eudyptula challenge
>
> Signed-off-by: AndyS 

Use your real name, please.

> ---
>  drivers/staging/ks7010/ks_wlan_net.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/ks7010/ks_wlan_net.c 
> b/drivers/staging/ks7010/ks_wlan_net.c
> index 8aa12e813bd7..e176876665df 100644
> --- a/drivers/staging/ks7010/ks_wlan_net.c
> +++ b/drivers/staging/ks7010/ks_wlan_net.c
> @@ -208,7 +208,7 @@ static int ks_wlan_set_freq(struct net_device *dev,
> /* for SLEEP MODE */
> /* If setting by frequency, convert to a channel */
> if ((fwrq->e == 1) &&
> -   (fwrq->m >= (int)2.412e8) && (fwrq->m <= (int)2.487e8)) {
> +   (fwrq->m >= 2.412e8) && (fwrq->m <= 2.487e8)) {

Do we really want to compare against a double value?

Frans

Re: [PATCH] Staging:ks7010:ks_wlan_net.c: unneeded type casting removed

[Frans Klaver]

On Mon, Jul 10, 2017 at 9:09 AM, AndyS  wrote:
> Subject: [PATCH] Staging:ks7010:ks_wlan_net.c: unneeded type casting removed

[PATCH] staging: ks7010: remove unneeded type casting

> removed undesired type casting. Warning was raised by checkpatch.pl
> This patch is for eudyptula challenge
>
> Signed-off-by: AndyS 

Use your real name, please.

> ---
>  drivers/staging/ks7010/ks_wlan_net.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/ks7010/ks_wlan_net.c 
> b/drivers/staging/ks7010/ks_wlan_net.c
> index 8aa12e813bd7..e176876665df 100644
> --- a/drivers/staging/ks7010/ks_wlan_net.c
> +++ b/drivers/staging/ks7010/ks_wlan_net.c
> @@ -208,7 +208,7 @@ static int ks_wlan_set_freq(struct net_device *dev,
> /* for SLEEP MODE */
> /* If setting by frequency, convert to a channel */
> if ((fwrq->e == 1) &&
> -   (fwrq->m >= (int)2.412e8) && (fwrq->m <= (int)2.487e8)) {
> +   (fwrq->m >= 2.412e8) && (fwrq->m <= 2.487e8)) {

Do we really want to compare against a double value?

Frans

Re: [greybus-dev] [PATCH v2] staging: greybus: arche: wrap over-length lines

[Frans Klaver]

On Mon, Jul 10, 2017 at 6:46 AM, Viresh Kumar  wrote:
> Hi Mitchell,
>
> On 09-07-17, 20:25, Mitchell Tasman wrote:
>> Adjust formatting of various statements to keep line length within
>> the 80 column limit preferred by the Linux kernel coding style.
>
> We try to follow that most of the time, but the end result should be easily
> readable.  If it isn't, then we just ignore the rule.
>
>> Signed-off-by: Mitchell Tasman 
>> ---
>> Changes in v2: Add back a missing space in a comment
>>
>>  drivers/staging/greybus/arche-platform.c | 29 +++--
>>  1 file changed, 19 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/staging/greybus/arche-platform.c 
>> b/drivers/staging/greybus/arche-platform.c
>> index eced2d2..eedd239 100644
>> --- a/drivers/staging/greybus/arche-platform.c
>> +++ b/drivers/staging/greybus/arche-platform.c
>> @@ -172,15 +172,21 @@ static irqreturn_t arche_platform_wd_irq(int irq, void 
>> *devid)
>>   if (arche_pdata->wake_detect_state == WD_STATE_BOOT_INIT) {
>>   if (time_before(jiffies,
>>   arche_pdata->wake_detect_start +
>> - 
>> msecs_to_jiffies(WD_COLDBOOT_PULSE_WIDTH_MS))) {
>> - 
>> arche_platform_set_wake_detect_state(arche_pdata,
>> -  
>> WD_STATE_IDLE);
>> + msecs_to_jiffies(
>> + WD_COLDBOOT_PULSE_WIDTH_MS))) {
>> + arche_platform_set_wake_detect_state(
>
> We don't break the lines like this in kernel to satisfy the 80 column width
> rule. Surely, some places would have similar code, but in general this isn't
> recommended. And that's why we never bothered about 80 column rule in this and
> below cases.

In cases like this, one could argue that you should start looking at
the level of indentation, rather than the line length per se. After
all, the documentation states that "if you need more than 3 levels of
indentation, you're screwed anyway, and you should fix your program".

Re: [greybus-dev] [PATCH v2] staging: greybus: arche: wrap over-length lines

[Frans Klaver]

On Mon, Jul 10, 2017 at 6:46 AM, Viresh Kumar  wrote:
> Hi Mitchell,
>
> On 09-07-17, 20:25, Mitchell Tasman wrote:
>> Adjust formatting of various statements to keep line length within
>> the 80 column limit preferred by the Linux kernel coding style.
>
> We try to follow that most of the time, but the end result should be easily
> readable.  If it isn't, then we just ignore the rule.
>
>> Signed-off-by: Mitchell Tasman 
>> ---
>> Changes in v2: Add back a missing space in a comment
>>
>>  drivers/staging/greybus/arche-platform.c | 29 +++--
>>  1 file changed, 19 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/staging/greybus/arche-platform.c 
>> b/drivers/staging/greybus/arche-platform.c
>> index eced2d2..eedd239 100644
>> --- a/drivers/staging/greybus/arche-platform.c
>> +++ b/drivers/staging/greybus/arche-platform.c
>> @@ -172,15 +172,21 @@ static irqreturn_t arche_platform_wd_irq(int irq, void 
>> *devid)
>>   if (arche_pdata->wake_detect_state == WD_STATE_BOOT_INIT) {
>>   if (time_before(jiffies,
>>   arche_pdata->wake_detect_start +
>> - 
>> msecs_to_jiffies(WD_COLDBOOT_PULSE_WIDTH_MS))) {
>> - 
>> arche_platform_set_wake_detect_state(arche_pdata,
>> -  
>> WD_STATE_IDLE);
>> + msecs_to_jiffies(
>> + WD_COLDBOOT_PULSE_WIDTH_MS))) {
>> + arche_platform_set_wake_detect_state(
>
> We don't break the lines like this in kernel to satisfy the 80 column width
> rule. Surely, some places would have similar code, but in general this isn't
> recommended. And that's why we never bothered about 80 column rule in this and
> below cases.

In cases like this, one could argue that you should start looking at
the level of indentation, rather than the line length per se. After
all, the documentation states that "if you need more than 3 levels of
indentation, you're screwed anyway, and you should fix your program".

Re: [PATCH 2/2] Staging: dgnc: I have fixed the changes in dgnc_neo.c This is a patch to the dgnc_neo.c warning udealy to usleep range Signed-off-by: Yash Omer <yashomer0...@gmail.com>

[Frans Klaver]

Hi,

On Thu, Jul 6, 2017 at 3:49 AM, yash007  wrote:
> From: Yash Omer 

Instead of resending the same thing every time, could you instead fix
your commit message and send a v2?

Also, where's patch 1 of 2?

>
> ---
>  drivers/staging/dgnc/dgnc_neo.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/dgnc/dgnc_neo.c b/drivers/staging/dgnc/dgnc_neo.c
> index 1943e66..0034ebe 100644
> --- a/drivers/staging/dgnc/dgnc_neo.c
> +++ b/drivers/staging/dgnc/dgnc_neo.c
> @@ -1230,7 +1230,7 @@ static void neo_flush_uart_write(struct channel_t *ch)
>  */
> tmp = readb(>ch_neo_uart->isr_fcr);
> if (tmp & 4)
> -   udelay(10);
> +   usleep_range(10);
> else
> break;
> }
> @@ -1261,7 +1261,7 @@ static void neo_flush_uart_read(struct channel_t *ch)
>  */
> tmp = readb(>ch_neo_uart->isr_fcr);
> if (tmp & 2)
> -   udelay(10);
> +   usleep_range(10);
> else
> break;
> }
> @@ -1483,7 +1483,7 @@ static void neo_assert_modem_signals(struct channel_t 
> *ch)
> neo_pci_posting_flush(ch->ch_bd);
>
> /* Give time for the UART to actually raise/drop the signals */
> -   udelay(10);
> +   usleep_range(10);
>  }
>
>  static void neo_send_start_character(struct channel_t *ch)
> @@ -1495,7 +1495,7 @@ static void neo_send_start_character(struct channel_t 
> *ch)
> ch->ch_xon_sends++;
> writeb(ch->ch_startc, >ch_neo_uart->txrx);
> neo_pci_posting_flush(ch->ch_bd);
> -   udelay(10);
> +   usleep_range(10);
> }
>  }
>
> @@ -1508,7 +1508,7 @@ static void neo_send_stop_character(struct channel_t 
> *ch)
> ch->ch_xoff_sends++;
> writeb(ch->ch_stopc, >ch_neo_uart->txrx);
> neo_pci_posting_flush(ch->ch_bd);
> -   udelay(10);
> +   usleep_range(10);
> }
>  }
>
> --
> 2.1.4
>

Re: [PATCH 2/2] Staging: dgnc: I have fixed the changes in dgnc_neo.c This is a patch to the dgnc_neo.c warning udealy to usleep range Signed-off-by: Yash Omer

[Frans Klaver]

Hi,

On Thu, Jul 6, 2017 at 3:49 AM, yash007  wrote:
> From: Yash Omer 

Instead of resending the same thing every time, could you instead fix
your commit message and send a v2?

Also, where's patch 1 of 2?

>
> ---
>  drivers/staging/dgnc/dgnc_neo.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/dgnc/dgnc_neo.c b/drivers/staging/dgnc/dgnc_neo.c
> index 1943e66..0034ebe 100644
> --- a/drivers/staging/dgnc/dgnc_neo.c
> +++ b/drivers/staging/dgnc/dgnc_neo.c
> @@ -1230,7 +1230,7 @@ static void neo_flush_uart_write(struct channel_t *ch)
>  */
> tmp = readb(>ch_neo_uart->isr_fcr);
> if (tmp & 4)
> -   udelay(10);
> +   usleep_range(10);
> else
> break;
> }
> @@ -1261,7 +1261,7 @@ static void neo_flush_uart_read(struct channel_t *ch)
>  */
> tmp = readb(>ch_neo_uart->isr_fcr);
> if (tmp & 2)
> -   udelay(10);
> +   usleep_range(10);
> else
> break;
> }
> @@ -1483,7 +1483,7 @@ static void neo_assert_modem_signals(struct channel_t 
> *ch)
> neo_pci_posting_flush(ch->ch_bd);
>
> /* Give time for the UART to actually raise/drop the signals */
> -   udelay(10);
> +   usleep_range(10);
>  }
>
>  static void neo_send_start_character(struct channel_t *ch)
> @@ -1495,7 +1495,7 @@ static void neo_send_start_character(struct channel_t 
> *ch)
> ch->ch_xon_sends++;
> writeb(ch->ch_startc, >ch_neo_uart->txrx);
> neo_pci_posting_flush(ch->ch_bd);
> -   udelay(10);
> +   usleep_range(10);
> }
>  }
>
> @@ -1508,7 +1508,7 @@ static void neo_send_stop_character(struct channel_t 
> *ch)
> ch->ch_xoff_sends++;
> writeb(ch->ch_stopc, >ch_neo_uart->txrx);
> neo_pci_posting_flush(ch->ch_bd);
> -   udelay(10);
> +   usleep_range(10);
> }
>  }
>
> --
> 2.1.4
>

Re: [PATCH 8/8] Staging: lustre :lustre: include :lustre_compat.h: Prefer using the BIT macro

[Frans Klaver]

On Thu, Jul 6, 2017 at 9:13 AM, Jaya Durga  wrote:
> Subject: Staging: lustre :lustre: include :lustre_compat.h: Prefer using the 
> BIT macro

Don't overdo it ;-).

Subject: staging: lustre: lustre_compat.h: Prefer using the BIT macro

> Replace all instances of (1 << 27) with BIT(27) to fix
> checkpatch check messages

While it may technically be true that this one instance is every
instance of (1<<27) there is in lustre_compat.h, I must say I expected
a bigger patch when I saw "replace all instances".


> Signed-off-by: Jaya Durga 
> ---
>  drivers/staging/lustre/lustre/include/lustre_compat.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/lustre/lustre/include/lustre_compat.h 
> b/drivers/staging/lustre/lustre/include/lustre_compat.h
> index da9ce19..686a251 100644
> --- a/drivers/staging/lustre/lustre/include/lustre_compat.h
> +++ b/drivers/staging/lustre/lustre/include/lustre_compat.h
> @@ -43,7 +43,7 @@
>   * set ATTR_BLOCKS to a high value to avoid any risk of collision with other
>   * ATTR_* attributes (see bug 13828)
>   */
> -#define ATTR_BLOCKS(1 << 27)
> +#define ATTR_BLOCKSBIT(27)
>
>  #define current_ngroups current_cred()->group_info->ngroups
>  #define current_groups current_cred()->group_info->small_block
> --
> 1.9.1
>
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 8/8] Staging: lustre :lustre: include :lustre_compat.h: Prefer using the BIT macro

[Frans Klaver]

On Thu, Jul 6, 2017 at 9:13 AM, Jaya Durga  wrote:
> Subject: Staging: lustre :lustre: include :lustre_compat.h: Prefer using the 
> BIT macro

Don't overdo it ;-).

Subject: staging: lustre: lustre_compat.h: Prefer using the BIT macro

> Replace all instances of (1 << 27) with BIT(27) to fix
> checkpatch check messages

While it may technically be true that this one instance is every
instance of (1<<27) there is in lustre_compat.h, I must say I expected
a bigger patch when I saw "replace all instances".


> Signed-off-by: Jaya Durga 
> ---
>  drivers/staging/lustre/lustre/include/lustre_compat.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/lustre/lustre/include/lustre_compat.h 
> b/drivers/staging/lustre/lustre/include/lustre_compat.h
> index da9ce19..686a251 100644
> --- a/drivers/staging/lustre/lustre/include/lustre_compat.h
> +++ b/drivers/staging/lustre/lustre/include/lustre_compat.h
> @@ -43,7 +43,7 @@
>   * set ATTR_BLOCKS to a high value to avoid any risk of collision with other
>   * ATTR_* attributes (see bug 13828)
>   */
> -#define ATTR_BLOCKS(1 << 27)
> +#define ATTR_BLOCKSBIT(27)
>
>  #define current_ngroups current_cred()->group_info->ngroups
>  #define current_groups current_cred()->group_info->small_block
> --
> 1.9.1
>
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] Staging: dgnc: I have fixed the changes in dgnc_neo.c This is a patch to the dgnc_neo.c warning udealy to usleep range Signed-off-by: Yash Omer <yashomer0...@gmail.com>

[Frans Klaver]

On Thu, Jul 6, 2017 at 3:22 AM, yash007  wrote:
> From: Yash Omer 

Your commit message is completely broken. Please fix it. See
Documentation/process/submitting-patches.rst chapter 14.

>
> ---
>  drivers/staging/dgnc/dgnc_neo.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/dgnc/dgnc_neo.c b/drivers/staging/dgnc/dgnc_neo.c
> index 1943e66..0034ebe 100644
> --- a/drivers/staging/dgnc/dgnc_neo.c
> +++ b/drivers/staging/dgnc/dgnc_neo.c
> @@ -1230,7 +1230,7 @@ static void neo_flush_uart_write(struct channel_t *ch)
>  */
> tmp = readb(>ch_neo_uart->isr_fcr);
> if (tmp & 4)
> -   udelay(10);
> +   usleep_range(10);
> else
> break;
> }
> @@ -1261,7 +1261,7 @@ static void neo_flush_uart_read(struct channel_t *ch)
>  */
> tmp = readb(>ch_neo_uart->isr_fcr);
> if (tmp & 2)
> -   udelay(10);
> +   usleep_range(10);
> else
> break;
> }
> @@ -1483,7 +1483,7 @@ static void neo_assert_modem_signals(struct channel_t 
> *ch)
> neo_pci_posting_flush(ch->ch_bd);
>
> /* Give time for the UART to actually raise/drop the signals */
> -   udelay(10);
> +   usleep_range(10);
>  }
>
>  static void neo_send_start_character(struct channel_t *ch)
> @@ -1495,7 +1495,7 @@ static void neo_send_start_character(struct channel_t 
> *ch)
> ch->ch_xon_sends++;
> writeb(ch->ch_startc, >ch_neo_uart->txrx);
> neo_pci_posting_flush(ch->ch_bd);
> -   udelay(10);
> +   usleep_range(10);
> }
>  }
>
> @@ -1508,7 +1508,7 @@ static void neo_send_stop_character(struct channel_t 
> *ch)
> ch->ch_xoff_sends++;
> writeb(ch->ch_stopc, >ch_neo_uart->txrx);
> neo_pci_posting_flush(ch->ch_bd);
> -   udelay(10);
> +   usleep_range(10);
> }
>  }
>
> --
> 2.1.4
>

Re: [PATCH 2/2] Staging: dgnc: I have fixed the changes in dgnc_neo.c This is a patch to the dgnc_neo.c warning udealy to usleep range Signed-off-by: Yash Omer

[Frans Klaver]

On Thu, Jul 6, 2017 at 3:22 AM, yash007  wrote:
> From: Yash Omer 

Your commit message is completely broken. Please fix it. See
Documentation/process/submitting-patches.rst chapter 14.

>
> ---
>  drivers/staging/dgnc/dgnc_neo.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/dgnc/dgnc_neo.c b/drivers/staging/dgnc/dgnc_neo.c
> index 1943e66..0034ebe 100644
> --- a/drivers/staging/dgnc/dgnc_neo.c
> +++ b/drivers/staging/dgnc/dgnc_neo.c
> @@ -1230,7 +1230,7 @@ static void neo_flush_uart_write(struct channel_t *ch)
>  */
> tmp = readb(>ch_neo_uart->isr_fcr);
> if (tmp & 4)
> -   udelay(10);
> +   usleep_range(10);
> else
> break;
> }
> @@ -1261,7 +1261,7 @@ static void neo_flush_uart_read(struct channel_t *ch)
>  */
> tmp = readb(>ch_neo_uart->isr_fcr);
> if (tmp & 2)
> -   udelay(10);
> +   usleep_range(10);
> else
> break;
> }
> @@ -1483,7 +1483,7 @@ static void neo_assert_modem_signals(struct channel_t 
> *ch)
> neo_pci_posting_flush(ch->ch_bd);
>
> /* Give time for the UART to actually raise/drop the signals */
> -   udelay(10);
> +   usleep_range(10);
>  }
>
>  static void neo_send_start_character(struct channel_t *ch)
> @@ -1495,7 +1495,7 @@ static void neo_send_start_character(struct channel_t 
> *ch)
> ch->ch_xon_sends++;
> writeb(ch->ch_startc, >ch_neo_uart->txrx);
> neo_pci_posting_flush(ch->ch_bd);
> -   udelay(10);
> +   usleep_range(10);
> }
>  }
>
> @@ -1508,7 +1508,7 @@ static void neo_send_stop_character(struct channel_t 
> *ch)
> ch->ch_xoff_sends++;
> writeb(ch->ch_stopc, >ch_neo_uart->txrx);
> neo_pci_posting_flush(ch->ch_bd);
> -   udelay(10);
> +   usleep_range(10);
> }
>  }
>
> --
> 2.1.4
>

Re: [tip:sched/core] sched/cputime: Refactor the cputime_adjust() code

[Frans Klaver]

On Fri, Jun 30, 2017 at 4:00 PM, Rik van Riel  wrote:
> On Fri, 2017-06-30 at 06:10 -0700, tip-bot for Gustavo A. R. Silva
> wrote:
>
>> +++ b/kernel/sched/cputime.c
>> @@ -615,19 +615,13 @@ static void cputime_adjust(struct task_cputime
>> *curr,
>>* userspace. Once a task gets some ticks, the monotonicy
>> code at
>>* 'update' will ensure things converge to the observed
>> ratio.
>>*/
>> - if (stime == 0) {
>> - utime = rtime;
>> - goto update;
>> + if (stime != 0) {
>> + if (utime == 0)
>> + stime = rtime;
>> + else
>> + stime = scale_stime(stime, rtime, stime +
>> utime);
>>   }
>>
>> - if (utime == 0) {
>> - stime = rtime;
>> - goto update;
>> - }
>> -
>> - stime = scale_stime(stime, rtime, stime + utime);
>> -
>> -update:
>
> Wait, what?
>
> This get rid of the utime = rtime assignment, when
> stime == 0.  That could be a correctness issue.

The first time utime is used after that assignment, it is overwritten
with rtime - stime. The utime = rtime assignment is then pointless.

Re: [tip:sched/core] sched/cputime: Refactor the cputime_adjust() code

[Frans Klaver]

On Fri, Jun 30, 2017 at 4:00 PM, Rik van Riel  wrote:
> On Fri, 2017-06-30 at 06:10 -0700, tip-bot for Gustavo A. R. Silva
> wrote:
>
>> +++ b/kernel/sched/cputime.c
>> @@ -615,19 +615,13 @@ static void cputime_adjust(struct task_cputime
>> *curr,
>>* userspace. Once a task gets some ticks, the monotonicy
>> code at
>>* 'update' will ensure things converge to the observed
>> ratio.
>>*/
>> - if (stime == 0) {
>> - utime = rtime;
>> - goto update;
>> + if (stime != 0) {
>> + if (utime == 0)
>> + stime = rtime;
>> + else
>> + stime = scale_stime(stime, rtime, stime +
>> utime);
>>   }
>>
>> - if (utime == 0) {
>> - stime = rtime;
>> - goto update;
>> - }
>> -
>> - stime = scale_stime(stime, rtime, stime + utime);
>> -
>> -update:
>
> Wait, what?
>
> This get rid of the utime = rtime assignment, when
> stime == 0.  That could be a correctness issue.

The first time utime is used after that assignment, it is overwritten
with rtime - stime. The utime = rtime assignment is then pointless.

Re: [PATCH] staging: ks7010: fix styling WARNINGs

[Frans Klaver]

On Fri, Jun 30, 2017 at 6:52 AM, Mark Rogers  wrote:
> Trivial style changes. There are still 3 "line over 80 characters"
> checkpatch.pl warnings, but I think they are best left alone as
> breaking the first two warning lines could hurt readability. The third
> warning is a message that should not be broken for the sake of grep.
>
> All but one of the changes fix lines that exceed 80 characters. An
> embedded function name was replaced by __func__ as well.
>
> Signed-off-by: Mark Rogers 
> ---
>  drivers/staging/ks7010/ks7010_sdio.c | 18 +++---
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
> b/drivers/staging/ks7010/ks7010_sdio.c
> index c325f48..6c0c6b2 100644
> --- a/drivers/staging/ks7010/ks7010_sdio.c
> +++ b/drivers/staging/ks7010/ks7010_sdio.c
> @@ -548,7 +548,8 @@ static void ks_sdio_interrupt(struct sdio_func *func)
> if (atomic_read(>psstatus.status) == PS_SNOOZE) 
> {
> if (cnt_txqbody(priv)) {
> ks_wlan_hw_wakeup_request(priv);
> -   queue_delayed_work(priv->wq, 
> >rw_dwork, 1);
> +   queue_delayed_work(priv->wq,
> +  >rw_dwork, 
> 1);
> return;
> }
> } else {
> @@ -693,15 +694,18 @@ static int ks7010_upload_firmware(struct ks_sdio_card 
> *card)
> memcpy(rom_buf, fw_entry->data + n, size);
>
> offset = n;
> -   ret = ks7010_sdio_update_index(priv, KS7010_IRAM_ADDRESS + 
> offset);
> +   ret = ks7010_sdio_update_index(priv,
> +  KS7010_IRAM_ADDRESS + offset);
> if (ret)
> goto release_firmware;
>
> -   ret = ks7010_sdio_write(priv, DATA_WINDOW, rom_buf, size);
> +   ret = ks7010_sdio_write(priv,
> +   DATA_WINDOW, rom_buf, size);
> if (ret)
> goto release_firmware;
>
> -   ret = ks7010_sdio_data_compare(priv, DATA_WINDOW, rom_buf, 
> size);
> +   ret = ks7010_sdio_data_compare(priv,
> +  DATA_WINDOW, rom_buf, size);
> if (ret)
> goto release_firmware;
>
> @@ -889,7 +893,7 @@ static int ks7010_sdio_probe(struct sdio_func *func,
> priv = netdev_priv(netdev);
>
> card->priv = priv;
> -   SET_NETDEV_DEV(netdev, >func->dev);   /* for create sysfs 
> symlinks */
> +   SET_NETDEV_DEV(netdev, >func->dev);/* for create sysfs symlinks 
> */

I don't think this is much of an improvement for readability. Should
we move the comment about a bit?

>
> /* private memory initialize */
> priv->ks_sdio_card = card;
> @@ -923,7 +927,7 @@ static int ks7010_sdio_probe(struct sdio_func *func,
> }
>
> /* interrupt setting */
> -   /* clear Interrupt status write (ARMtoSD_InterruptPending 
> FN1:00_0024) */
> +   /* clear Interrupt status write (ARMtoSD_InterruptPending 
> FN1:00_0024)*/

This is a bit of a pointless change, isn't it? It also makes the comment uglier.

> sdio_claim_host(func);
> ret = ks7010_sdio_writeb(priv, INT_PENDING, 0xff);
> sdio_release_host(func);
> @@ -1006,7 +1010,7 @@ static void ks7010_sdio_remove(struct sdio_func *func)
> struct ks_sdio_card *card;
> struct ks_wlan_private *priv;
>
> -   DPRINTK(1, "ks7010_sdio_remove()\n");
> +   DPRINTK(1, "%s()\n", __func__);

You might get a "one thing per patch please" for this. You wouldn't
have had to change this line if you'd strictly stuck to that.

Frans

Re: [PATCH] staging: ks7010: fix styling WARNINGs

[Frans Klaver]

On Fri, Jun 30, 2017 at 6:52 AM, Mark Rogers  wrote:
> Trivial style changes. There are still 3 "line over 80 characters"
> checkpatch.pl warnings, but I think they are best left alone as
> breaking the first two warning lines could hurt readability. The third
> warning is a message that should not be broken for the sake of grep.
>
> All but one of the changes fix lines that exceed 80 characters. An
> embedded function name was replaced by __func__ as well.
>
> Signed-off-by: Mark Rogers 
> ---
>  drivers/staging/ks7010/ks7010_sdio.c | 18 +++---
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
> b/drivers/staging/ks7010/ks7010_sdio.c
> index c325f48..6c0c6b2 100644
> --- a/drivers/staging/ks7010/ks7010_sdio.c
> +++ b/drivers/staging/ks7010/ks7010_sdio.c
> @@ -548,7 +548,8 @@ static void ks_sdio_interrupt(struct sdio_func *func)
> if (atomic_read(>psstatus.status) == PS_SNOOZE) 
> {
> if (cnt_txqbody(priv)) {
> ks_wlan_hw_wakeup_request(priv);
> -   queue_delayed_work(priv->wq, 
> >rw_dwork, 1);
> +   queue_delayed_work(priv->wq,
> +  >rw_dwork, 
> 1);
> return;
> }
> } else {
> @@ -693,15 +694,18 @@ static int ks7010_upload_firmware(struct ks_sdio_card 
> *card)
> memcpy(rom_buf, fw_entry->data + n, size);
>
> offset = n;
> -   ret = ks7010_sdio_update_index(priv, KS7010_IRAM_ADDRESS + 
> offset);
> +   ret = ks7010_sdio_update_index(priv,
> +  KS7010_IRAM_ADDRESS + offset);
> if (ret)
> goto release_firmware;
>
> -   ret = ks7010_sdio_write(priv, DATA_WINDOW, rom_buf, size);
> +   ret = ks7010_sdio_write(priv,
> +   DATA_WINDOW, rom_buf, size);
> if (ret)
> goto release_firmware;
>
> -   ret = ks7010_sdio_data_compare(priv, DATA_WINDOW, rom_buf, 
> size);
> +   ret = ks7010_sdio_data_compare(priv,
> +  DATA_WINDOW, rom_buf, size);
> if (ret)
> goto release_firmware;
>
> @@ -889,7 +893,7 @@ static int ks7010_sdio_probe(struct sdio_func *func,
> priv = netdev_priv(netdev);
>
> card->priv = priv;
> -   SET_NETDEV_DEV(netdev, >func->dev);   /* for create sysfs 
> symlinks */
> +   SET_NETDEV_DEV(netdev, >func->dev);/* for create sysfs symlinks 
> */

I don't think this is much of an improvement for readability. Should
we move the comment about a bit?

>
> /* private memory initialize */
> priv->ks_sdio_card = card;
> @@ -923,7 +927,7 @@ static int ks7010_sdio_probe(struct sdio_func *func,
> }
>
> /* interrupt setting */
> -   /* clear Interrupt status write (ARMtoSD_InterruptPending 
> FN1:00_0024) */
> +   /* clear Interrupt status write (ARMtoSD_InterruptPending 
> FN1:00_0024)*/

This is a bit of a pointless change, isn't it? It also makes the comment uglier.

> sdio_claim_host(func);
> ret = ks7010_sdio_writeb(priv, INT_PENDING, 0xff);
> sdio_release_host(func);
> @@ -1006,7 +1010,7 @@ static void ks7010_sdio_remove(struct sdio_func *func)
> struct ks_sdio_card *card;
> struct ks_wlan_private *priv;
>
> -   DPRINTK(1, "ks7010_sdio_remove()\n");
> +   DPRINTK(1, "%s()\n", __func__);

You might get a "one thing per patch please" for this. You wouldn't
have had to change this line if you'd strictly stuck to that.

Frans

Re: [PATCH] clocksource: em_sti: fix error return codes in em_sti_probe()

[Frans Klaver]

On Fri, Jun 30, 2017 at 6:42 AM, Gustavo A. R. Silva
 wrote:
> Propagate the return values of platform_get_irq and
> devm_request_irq on failure.
>
> Signed-off-by: Gustavo A. R. Silva 
> ---
>  drivers/clocksource/em_sti.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clocksource/em_sti.c b/drivers/clocksource/em_sti.c
> index bc48cbf..c4818dd 100644
> --- a/drivers/clocksource/em_sti.c
> +++ b/drivers/clocksource/em_sti.c
> @@ -305,7 +305,7 @@ static int em_sti_probe(struct platform_device *pdev)
> irq = platform_get_irq(pdev, 0);
> if (irq < 0) {
> dev_err(>dev, "failed to get irq\n");
> -   return -EINVAL;
> +   return irq;
> }
>
> /* map memory, let base point to the STI instance */
> @@ -314,11 +314,12 @@ static int em_sti_probe(struct platform_device *pdev)
> if (IS_ERR(p->base))
> return PTR_ERR(p->base);
>
> -   if (devm_request_irq(>dev, irq, em_sti_interrupt,
> +   irq = devm_request_irq(>dev, irq, em_sti_interrupt,
>  IRQF_TIMER | IRQF_IRQPOLL | IRQF_NOBALANCING,
> -dev_name(>dev), p)) {
> +dev_name(>dev), p);
> +   if (irq) {
> dev_err(>dev, "failed to request low IRQ\n");
> -   return -ENOENT;
> +   return irq;
> }

This works. Yet I think that 'ret' would be a better candidate for
taking the result of devm_request_irq, since it doesn't return the irq
number on success. Should someone decide to reference irq at a later
point in the code, this has to be changed.

Re: [PATCH] clocksource: em_sti: fix error return codes in em_sti_probe()

[Frans Klaver]

On Fri, Jun 30, 2017 at 6:42 AM, Gustavo A. R. Silva
 wrote:
> Propagate the return values of platform_get_irq and
> devm_request_irq on failure.
>
> Signed-off-by: Gustavo A. R. Silva 
> ---
>  drivers/clocksource/em_sti.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clocksource/em_sti.c b/drivers/clocksource/em_sti.c
> index bc48cbf..c4818dd 100644
> --- a/drivers/clocksource/em_sti.c
> +++ b/drivers/clocksource/em_sti.c
> @@ -305,7 +305,7 @@ static int em_sti_probe(struct platform_device *pdev)
> irq = platform_get_irq(pdev, 0);
> if (irq < 0) {
> dev_err(>dev, "failed to get irq\n");
> -   return -EINVAL;
> +   return irq;
> }
>
> /* map memory, let base point to the STI instance */
> @@ -314,11 +314,12 @@ static int em_sti_probe(struct platform_device *pdev)
> if (IS_ERR(p->base))
> return PTR_ERR(p->base);
>
> -   if (devm_request_irq(>dev, irq, em_sti_interrupt,
> +   irq = devm_request_irq(>dev, irq, em_sti_interrupt,
>  IRQF_TIMER | IRQF_IRQPOLL | IRQF_NOBALANCING,
> -dev_name(>dev), p)) {
> +dev_name(>dev), p);
> +   if (irq) {
> dev_err(>dev, "failed to request low IRQ\n");
> -   return -ENOENT;
> +   return irq;
> }

This works. Yet I think that 'ret' would be a better candidate for
taking the result of devm_request_irq, since it doesn't return the irq
number on success. Should someone decide to reference irq at a later
point in the code, this has to be changed.

Re: [PATCH 3/6] staging: iio: tsl2x7x: remove tsl2x7x_i2c_read()

[Frans Klaver]

On Thu, Jun 29, 2017 at 7:03 PM, Brian Masney  wrote:
> tsl2x7x_i2c_read() would call i2c_smbus_write_byte() and
> i2c_smbus_read_byte(). These two i2c functions can be replaced with a
> single call to i2c_smbus_read_byte_data(). This patch removes the
> tsl2x7x_i2c_read() function and replaces all occurances with a call to
> i2c_smbus_read_byte_data().

s,occurances,occurrences,

Frans

Re: [PATCH 3/6] staging: iio: tsl2x7x: remove tsl2x7x_i2c_read()

[Frans Klaver]

On Thu, Jun 29, 2017 at 7:03 PM, Brian Masney  wrote:
> tsl2x7x_i2c_read() would call i2c_smbus_write_byte() and
> i2c_smbus_read_byte(). These two i2c functions can be replaced with a
> single call to i2c_smbus_read_byte_data(). This patch removes the
> tsl2x7x_i2c_read() function and replaces all occurances with a call to
> i2c_smbus_read_byte_data().

s,occurances,occurrences,

Frans

Re: [PATCH 14/14] staging: ccree: fix block comment style

[Frans Klaver]

On Tue, Jun 27, 2017 at 9:27 AM, Gilad Ben-Yossef  wrote:
> Align block comments according to coding style.
>
> Signed-off-by: Gilad Ben-Yossef 
> ---
>  drivers/staging/ccree/cc_hw_queue_defs.h | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/ccree/cc_hw_queue_defs.h 
> b/drivers/staging/ccree/cc_hw_queue_defs.h
> index f11487a..e6b8cea 100644
> --- a/drivers/staging/ccree/cc_hw_queue_defs.h
> +++ b/drivers/staging/ccree/cc_hw_queue_defs.h
> @@ -23,8 +23,8 @@
>  #include 
>
>  
> /**
> -*  DEFINITIONS
> -**/
> + * DEFINITIONS
> + 
> **/

I think if you change to the preferred block comment format, you
should also drop these lines full of asterisks. I'm not sure why a
multi-line comment is warranted here, anyway.

Frans

Re: [PATCH 14/14] staging: ccree: fix block comment style

[Frans Klaver]

On Tue, Jun 27, 2017 at 9:27 AM, Gilad Ben-Yossef  wrote:
> Align block comments according to coding style.
>
> Signed-off-by: Gilad Ben-Yossef 
> ---
>  drivers/staging/ccree/cc_hw_queue_defs.h | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/ccree/cc_hw_queue_defs.h 
> b/drivers/staging/ccree/cc_hw_queue_defs.h
> index f11487a..e6b8cea 100644
> --- a/drivers/staging/ccree/cc_hw_queue_defs.h
> +++ b/drivers/staging/ccree/cc_hw_queue_defs.h
> @@ -23,8 +23,8 @@
>  #include 
>
>  
> /**
> -*  DEFINITIONS
> -**/
> + * DEFINITIONS
> + 
> **/

I think if you change to the preferred block comment format, you
should also drop these lines full of asterisks. I'm not sure why a
multi-line comment is warranted here, anyway.

Frans

Re: [PATCH 6/6] Staging: wlan-ng: hfa384x.h:Fix use of volatile is usually wrong

[Frans Klaver]

On Thu, Jun 29, 2017 at 12:25 PM, Jaya Durga  wrote:
> Fix checkpatch.pl issue
> WARNING: Use of volatile is usually wrong:
> see Documentation/process/volatile-considered-harmful.rst

Now I've only had a very quick look at the code using this. Could you
elaborate on why just removing the volatile keyword is enough, and
that this isn't related to some smelly bit of code that should be
implemented differently?


> Signed-off-by: Jaya Durga 
> ---
>  drivers/staging/wlan-ng/hfa384x.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/wlan-ng/hfa384x.h 
> b/drivers/staging/wlan-ng/hfa384x.h
> index 310e2c4..015945f 100644
> --- a/drivers/staging/wlan-ng/hfa384x.h
> +++ b/drivers/staging/wlan-ng/hfa384x.h
> @@ -1175,7 +1175,7 @@ struct hfa384x_usbctlx {
> enum ctlx_state state;  /* Tracks running state */
>
> struct completion done;
> -   volatile int reapable;  /* Food for the reaper task */
> +   int reapable;   /* Food for the reaper task */
>
> ctlx_cmdcb_t cmdcb; /* Async command callback */
> ctlx_usercb_t usercb;   /* Async user callback, */
> --
> 1.9.1

Frans

Re: [PATCH 6/6] Staging: wlan-ng: hfa384x.h:Fix use of volatile is usually wrong

[Frans Klaver]

On Thu, Jun 29, 2017 at 12:25 PM, Jaya Durga  wrote:
> Fix checkpatch.pl issue
> WARNING: Use of volatile is usually wrong:
> see Documentation/process/volatile-considered-harmful.rst

Now I've only had a very quick look at the code using this. Could you
elaborate on why just removing the volatile keyword is enough, and
that this isn't related to some smelly bit of code that should be
implemented differently?


> Signed-off-by: Jaya Durga 
> ---
>  drivers/staging/wlan-ng/hfa384x.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/wlan-ng/hfa384x.h 
> b/drivers/staging/wlan-ng/hfa384x.h
> index 310e2c4..015945f 100644
> --- a/drivers/staging/wlan-ng/hfa384x.h
> +++ b/drivers/staging/wlan-ng/hfa384x.h
> @@ -1175,7 +1175,7 @@ struct hfa384x_usbctlx {
> enum ctlx_state state;  /* Tracks running state */
>
> struct completion done;
> -   volatile int reapable;  /* Food for the reaper task */
> +   int reapable;   /* Food for the reaper task */
>
> ctlx_cmdcb_t cmdcb; /* Async command callback */
> ctlx_usercb_t usercb;   /* Async user callback, */
> --
> 1.9.1

Frans

Re: [PATCH] hwmon: Add LTC2471/LTC2473 driver

[Frans Klaver]

On Thu, Jun 29, 2017 at 1:30 PM, Mike Looijmans <mike.looijm...@topic.nl> wrote:
> On 29-06-17 13:18, Frans Klaver wrote:
>>
>> On Thu, Jun 29, 2017 at 12:45 PM, Mike Looijmans
>> <mike.looijm...@topic.nl> wrote:
>>>
>>> The LTC2741 and LTC2473 are single voltage monitoring chips. The LTC2473
>>> is similar to the LTC2471 but outputs a signed differential value.
>>>
>>> Datasheet:
>>>http://cds.linear.com/docs/en/datasheet/24713fb.pdf
>>>
>>> Signed-off-by: Mike Looijmans <mike.looijm...@topic.nl>
>>> ---
>>>   drivers/hwmon/Kconfig   |  10 
>>>   drivers/hwmon/Makefile  |   1 +
>>>   drivers/hwmon/ltc2471.c | 127
>>> 
>>>   3 files changed, 138 insertions(+)
>>>   create mode 100644 drivers/hwmon/ltc2471.c
>>>
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index fbde226..c9a2a87 100644
>>> --- a/drivers/hwmon/Kconfig
>>> +++ b/drivers/hwmon/Kconfig
>>> @@ -673,6 +673,16 @@ config SENSORS_LINEAGE
>>>This driver can also be built as a module.  If so, the module
>>>will be called lineage-pem.
>>>
>>> +config SENSORS_LTC2471
>>> +   tristate "Linear Technology LTC2471 and LTC2473"
>>> +   depends on I2C
>>> +   help
>>> + If you say yes here you get support for Linear Technology
>>> LTC2471
>>> + and LTC2473 voltage monitors.
>>> +
>>> + This driver can also be built as a module. If so, the module
>>> will
>>> + be called ltc2471.
>>> +
>>>   config SENSORS_LTC2945
>>>  tristate "Linear Technology LTC2945"
>>>  depends on I2C
>>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>>> index 58cc3ac..6f60fe7 100644
>>> --- a/drivers/hwmon/Makefile
>>> +++ b/drivers/hwmon/Makefile
>>> @@ -99,6 +99,7 @@ obj-$(CONFIG_SENSORS_LM93)+= lm93.o
>>>   obj-$(CONFIG_SENSORS_LM95234)  += lm95234.o
>>>   obj-$(CONFIG_SENSORS_LM95241)  += lm95241.o
>>>   obj-$(CONFIG_SENSORS_LM95245)  += lm95245.o
>>> +obj-$(CONFIG_SENSORS_LTC2471)  += ltc2471.o
>>>   obj-$(CONFIG_SENSORS_LTC2945)  += ltc2945.o
>>>   obj-$(CONFIG_SENSORS_LTC2990)  += ltc2990.o
>>>   obj-$(CONFIG_SENSORS_LTC4151)  += ltc4151.o
>>> diff --git a/drivers/hwmon/ltc2471.c b/drivers/hwmon/ltc2471.c
>>> new file mode 100644
>>> index 000..17eaad8
>>> --- /dev/null
>>> +++ b/drivers/hwmon/ltc2471.c
>>> @@ -0,0 +1,127 @@
>>> +/*
>>> + * Driver for Linear Technology LTC2471 and LTC2473 voltage monitors
>>> + * The LTC2473 is identical to the 2471, but reports a differential
>>> signal.
>>> + *
>>> + * Copyright (C) 2017 Topic Embedded Products
>>> + * Author: Mike Looijmans <mike.looijm...@topic.nl>
>>> + *
>>> + * License: GPLv2
>>> + */
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +enum chips {
>>> +   ltc2471,
>>> +   ltc2473
>>> +};
>>> +
>>> +struct ltc2471_data {
>>> +   struct i2c_client *i2c;
>>> +   bool differential;
>>> +};
>>
>>
>> I haven't checked if there are similar drivers, but how about
>> ltc247x_data? It's a bit odd to tie all naming in this driver to one
>> chip and then support another one as well.
>
>
> Did a query on the linear site, found the LTC2470 and LTC2472 which appear
> to be the SPI versions of the same chip. So yes, using "ltc247x" may cause
> confusion when other drivers arrive. Linear solves this by naming the
> datasheet "ltc24712" but I think that'd be even more confusing.

OK.


>>
>>> +
>>> +/* Reference voltage is 1.25V */
>>> +#define LTC2471_VREF 1250
>>> +
>>> +/* Read two bytes from the I2C bus to obtain the ADC result */
>>> +static int ltc2471_get_value(struct i2c_client *i2c)
>>> +{
>>> +   int ret;
>>> +   __be16 buf;
>>> +
>>> +   ret = i2c_master_recv(i2c, (char *), 2);
>>> +   if (ret < 0)
>>> +   return ret;
>>> +   if (ret != 2)
>>> +   return -EIO;
>>> +
>>> +   /* MSB first */

Re: [PATCH] hwmon: Add LTC2471/LTC2473 driver

[Frans Klaver]

On Thu, Jun 29, 2017 at 1:30 PM, Mike Looijmans  wrote:
> On 29-06-17 13:18, Frans Klaver wrote:
>>
>> On Thu, Jun 29, 2017 at 12:45 PM, Mike Looijmans
>>  wrote:
>>>
>>> The LTC2741 and LTC2473 are single voltage monitoring chips. The LTC2473
>>> is similar to the LTC2471 but outputs a signed differential value.
>>>
>>> Datasheet:
>>>http://cds.linear.com/docs/en/datasheet/24713fb.pdf
>>>
>>> Signed-off-by: Mike Looijmans 
>>> ---
>>>   drivers/hwmon/Kconfig   |  10 
>>>   drivers/hwmon/Makefile  |   1 +
>>>   drivers/hwmon/ltc2471.c | 127
>>> 
>>>   3 files changed, 138 insertions(+)
>>>   create mode 100644 drivers/hwmon/ltc2471.c
>>>
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index fbde226..c9a2a87 100644
>>> --- a/drivers/hwmon/Kconfig
>>> +++ b/drivers/hwmon/Kconfig
>>> @@ -673,6 +673,16 @@ config SENSORS_LINEAGE
>>>This driver can also be built as a module.  If so, the module
>>>will be called lineage-pem.
>>>
>>> +config SENSORS_LTC2471
>>> +   tristate "Linear Technology LTC2471 and LTC2473"
>>> +   depends on I2C
>>> +   help
>>> + If you say yes here you get support for Linear Technology
>>> LTC2471
>>> + and LTC2473 voltage monitors.
>>> +
>>> + This driver can also be built as a module. If so, the module
>>> will
>>> + be called ltc2471.
>>> +
>>>   config SENSORS_LTC2945
>>>  tristate "Linear Technology LTC2945"
>>>  depends on I2C
>>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>>> index 58cc3ac..6f60fe7 100644
>>> --- a/drivers/hwmon/Makefile
>>> +++ b/drivers/hwmon/Makefile
>>> @@ -99,6 +99,7 @@ obj-$(CONFIG_SENSORS_LM93)+= lm93.o
>>>   obj-$(CONFIG_SENSORS_LM95234)  += lm95234.o
>>>   obj-$(CONFIG_SENSORS_LM95241)  += lm95241.o
>>>   obj-$(CONFIG_SENSORS_LM95245)  += lm95245.o
>>> +obj-$(CONFIG_SENSORS_LTC2471)  += ltc2471.o
>>>   obj-$(CONFIG_SENSORS_LTC2945)  += ltc2945.o
>>>   obj-$(CONFIG_SENSORS_LTC2990)  += ltc2990.o
>>>   obj-$(CONFIG_SENSORS_LTC4151)  += ltc4151.o
>>> diff --git a/drivers/hwmon/ltc2471.c b/drivers/hwmon/ltc2471.c
>>> new file mode 100644
>>> index 000..17eaad8
>>> --- /dev/null
>>> +++ b/drivers/hwmon/ltc2471.c
>>> @@ -0,0 +1,127 @@
>>> +/*
>>> + * Driver for Linear Technology LTC2471 and LTC2473 voltage monitors
>>> + * The LTC2473 is identical to the 2471, but reports a differential
>>> signal.
>>> + *
>>> + * Copyright (C) 2017 Topic Embedded Products
>>> + * Author: Mike Looijmans 
>>> + *
>>> + * License: GPLv2
>>> + */
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +enum chips {
>>> +   ltc2471,
>>> +   ltc2473
>>> +};
>>> +
>>> +struct ltc2471_data {
>>> +   struct i2c_client *i2c;
>>> +   bool differential;
>>> +};
>>
>>
>> I haven't checked if there are similar drivers, but how about
>> ltc247x_data? It's a bit odd to tie all naming in this driver to one
>> chip and then support another one as well.
>
>
> Did a query on the linear site, found the LTC2470 and LTC2472 which appear
> to be the SPI versions of the same chip. So yes, using "ltc247x" may cause
> confusion when other drivers arrive. Linear solves this by naming the
> datasheet "ltc24712" but I think that'd be even more confusing.

OK.


>>
>>> +
>>> +/* Reference voltage is 1.25V */
>>> +#define LTC2471_VREF 1250
>>> +
>>> +/* Read two bytes from the I2C bus to obtain the ADC result */
>>> +static int ltc2471_get_value(struct i2c_client *i2c)
>>> +{
>>> +   int ret;
>>> +   __be16 buf;
>>> +
>>> +   ret = i2c_master_recv(i2c, (char *), 2);
>>> +   if (ret < 0)
>>> +   return ret;
>>> +   if (ret != 2)
>>> +   return -EIO;
>>> +
>>> +   /* MSB first */
>>> +   return be16_to_cpu(buf);
>>> +}
>>> +
>>> +static ssize_t ltc2471_show_value(struct

Re: [PATCH] hwmon: Add LTC2471/LTC2473 driver

[Frans Klaver]

On Thu, Jun 29, 2017 at 12:45 PM, Mike Looijmans
 wrote:
> The LTC2741 and LTC2473 are single voltage monitoring chips. The LTC2473
> is similar to the LTC2471 but outputs a signed differential value.
>
> Datasheet:
>   http://cds.linear.com/docs/en/datasheet/24713fb.pdf
>
> Signed-off-by: Mike Looijmans 
> ---
>  drivers/hwmon/Kconfig   |  10 
>  drivers/hwmon/Makefile  |   1 +
>  drivers/hwmon/ltc2471.c | 127 
> 
>  3 files changed, 138 insertions(+)
>  create mode 100644 drivers/hwmon/ltc2471.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index fbde226..c9a2a87 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -673,6 +673,16 @@ config SENSORS_LINEAGE
>   This driver can also be built as a module.  If so, the module
>   will be called lineage-pem.
>
> +config SENSORS_LTC2471
> +   tristate "Linear Technology LTC2471 and LTC2473"
> +   depends on I2C
> +   help
> + If you say yes here you get support for Linear Technology LTC2471
> + and LTC2473 voltage monitors.
> +
> + This driver can also be built as a module. If so, the module will
> + be called ltc2471.
> +
>  config SENSORS_LTC2945
> tristate "Linear Technology LTC2945"
> depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 58cc3ac..6f60fe7 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -99,6 +99,7 @@ obj-$(CONFIG_SENSORS_LM93)+= lm93.o
>  obj-$(CONFIG_SENSORS_LM95234)  += lm95234.o
>  obj-$(CONFIG_SENSORS_LM95241)  += lm95241.o
>  obj-$(CONFIG_SENSORS_LM95245)  += lm95245.o
> +obj-$(CONFIG_SENSORS_LTC2471)  += ltc2471.o
>  obj-$(CONFIG_SENSORS_LTC2945)  += ltc2945.o
>  obj-$(CONFIG_SENSORS_LTC2990)  += ltc2990.o
>  obj-$(CONFIG_SENSORS_LTC4151)  += ltc4151.o
> diff --git a/drivers/hwmon/ltc2471.c b/drivers/hwmon/ltc2471.c
> new file mode 100644
> index 000..17eaad8
> --- /dev/null
> +++ b/drivers/hwmon/ltc2471.c
> @@ -0,0 +1,127 @@
> +/*
> + * Driver for Linear Technology LTC2471 and LTC2473 voltage monitors
> + * The LTC2473 is identical to the 2471, but reports a differential signal.
> + *
> + * Copyright (C) 2017 Topic Embedded Products
> + * Author: Mike Looijmans 
> + *
> + * License: GPLv2
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +enum chips {
> +   ltc2471,
> +   ltc2473
> +};
> +
> +struct ltc2471_data {
> +   struct i2c_client *i2c;
> +   bool differential;
> +};

I haven't checked if there are similar drivers, but how about
ltc247x_data? It's a bit odd to tie all naming in this driver to one
chip and then support another one as well.

> +
> +/* Reference voltage is 1.25V */
> +#define LTC2471_VREF 1250
> +
> +/* Read two bytes from the I2C bus to obtain the ADC result */
> +static int ltc2471_get_value(struct i2c_client *i2c)
> +{
> +   int ret;
> +   __be16 buf;
> +
> +   ret = i2c_master_recv(i2c, (char *), 2);
> +   if (ret < 0)
> +   return ret;
> +   if (ret != 2)
> +   return -EIO;
> +
> +   /* MSB first */
> +   return be16_to_cpu(buf);
> +}
> +
> +static ssize_t ltc2471_show_value(struct device *dev,
> + struct device_attribute *da, char *buf)
> +{
> +   struct ltc2471_data *data = dev_get_drvdata(dev);
> +   int value;
> +
> +   value = ltc2471_get_value(data->i2c);
> +   if (unlikely(value < 0))
> +   return value;
> +
> +   if (data->differential)
> +   /* Ranges from -VREF to +VREF with "0" at 0x8000 */
> +   value = ((s32)LTC2471_VREF * (s32)(value - 0x8000)) >> 15;
> +   else
> +   /* Ranges from 0 to +VREF */
> +   value = ((u32)LTC2471_VREF * (u32)value) >> 16;
> +
> +   return snprintf(buf, PAGE_SIZE, "%d\n", value);
> +}
> +
> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ltc2471_show_value, NULL, 0);

Octal permissions are preferred over S_IRUGO and such.

> +
> +static struct attribute *ltc2471_attrs[] = {
> +   _dev_attr_in0_input.dev_attr.attr,
> +   NULL
> +};
> +
> +ATTRIBUTE_GROUPS(ltc2471);
> +
> +static int ltc2471_i2c_probe(struct i2c_client *i2c,
> +const struct i2c_device_id *id)
> +{
> +   int ret;
> +   struct device *hwmon_dev;
> +   struct ltc2471_data *data;
> +
> +   if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C))
> +   return -ENODEV;
> +
> +   data = devm_kzalloc(>dev, sizeof(struct ltc2471_data), 
> GFP_KERNEL);

sizeof(*data)

> +   if (unlikely(!data))
> +   return -ENOMEM;
> +
> +   data->i2c = i2c;
> +   data->differential = (id->driver_data == ltc2473);
> +
> +   /* Trigger once to start conversion and check if chip is there */
> +   ret = 

Re: [PATCH] hwmon: Add LTC2471/LTC2473 driver

[Frans Klaver]

On Thu, Jun 29, 2017 at 12:45 PM, Mike Looijmans
 wrote:
> The LTC2741 and LTC2473 are single voltage monitoring chips. The LTC2473
> is similar to the LTC2471 but outputs a signed differential value.
>
> Datasheet:
>   http://cds.linear.com/docs/en/datasheet/24713fb.pdf
>
> Signed-off-by: Mike Looijmans 
> ---
>  drivers/hwmon/Kconfig   |  10 
>  drivers/hwmon/Makefile  |   1 +
>  drivers/hwmon/ltc2471.c | 127 
> 
>  3 files changed, 138 insertions(+)
>  create mode 100644 drivers/hwmon/ltc2471.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index fbde226..c9a2a87 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -673,6 +673,16 @@ config SENSORS_LINEAGE
>   This driver can also be built as a module.  If so, the module
>   will be called lineage-pem.
>
> +config SENSORS_LTC2471
> +   tristate "Linear Technology LTC2471 and LTC2473"
> +   depends on I2C
> +   help
> + If you say yes here you get support for Linear Technology LTC2471
> + and LTC2473 voltage monitors.
> +
> + This driver can also be built as a module. If so, the module will
> + be called ltc2471.
> +
>  config SENSORS_LTC2945
> tristate "Linear Technology LTC2945"
> depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 58cc3ac..6f60fe7 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -99,6 +99,7 @@ obj-$(CONFIG_SENSORS_LM93)+= lm93.o
>  obj-$(CONFIG_SENSORS_LM95234)  += lm95234.o
>  obj-$(CONFIG_SENSORS_LM95241)  += lm95241.o
>  obj-$(CONFIG_SENSORS_LM95245)  += lm95245.o
> +obj-$(CONFIG_SENSORS_LTC2471)  += ltc2471.o
>  obj-$(CONFIG_SENSORS_LTC2945)  += ltc2945.o
>  obj-$(CONFIG_SENSORS_LTC2990)  += ltc2990.o
>  obj-$(CONFIG_SENSORS_LTC4151)  += ltc4151.o
> diff --git a/drivers/hwmon/ltc2471.c b/drivers/hwmon/ltc2471.c
> new file mode 100644
> index 000..17eaad8
> --- /dev/null
> +++ b/drivers/hwmon/ltc2471.c
> @@ -0,0 +1,127 @@
> +/*
> + * Driver for Linear Technology LTC2471 and LTC2473 voltage monitors
> + * The LTC2473 is identical to the 2471, but reports a differential signal.
> + *
> + * Copyright (C) 2017 Topic Embedded Products
> + * Author: Mike Looijmans 
> + *
> + * License: GPLv2
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +enum chips {
> +   ltc2471,
> +   ltc2473
> +};
> +
> +struct ltc2471_data {
> +   struct i2c_client *i2c;
> +   bool differential;
> +};

I haven't checked if there are similar drivers, but how about
ltc247x_data? It's a bit odd to tie all naming in this driver to one
chip and then support another one as well.

> +
> +/* Reference voltage is 1.25V */
> +#define LTC2471_VREF 1250
> +
> +/* Read two bytes from the I2C bus to obtain the ADC result */
> +static int ltc2471_get_value(struct i2c_client *i2c)
> +{
> +   int ret;
> +   __be16 buf;
> +
> +   ret = i2c_master_recv(i2c, (char *), 2);
> +   if (ret < 0)
> +   return ret;
> +   if (ret != 2)
> +   return -EIO;
> +
> +   /* MSB first */
> +   return be16_to_cpu(buf);
> +}
> +
> +static ssize_t ltc2471_show_value(struct device *dev,
> + struct device_attribute *da, char *buf)
> +{
> +   struct ltc2471_data *data = dev_get_drvdata(dev);
> +   int value;
> +
> +   value = ltc2471_get_value(data->i2c);
> +   if (unlikely(value < 0))
> +   return value;
> +
> +   if (data->differential)
> +   /* Ranges from -VREF to +VREF with "0" at 0x8000 */
> +   value = ((s32)LTC2471_VREF * (s32)(value - 0x8000)) >> 15;
> +   else
> +   /* Ranges from 0 to +VREF */
> +   value = ((u32)LTC2471_VREF * (u32)value) >> 16;
> +
> +   return snprintf(buf, PAGE_SIZE, "%d\n", value);
> +}
> +
> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ltc2471_show_value, NULL, 0);

Octal permissions are preferred over S_IRUGO and such.

> +
> +static struct attribute *ltc2471_attrs[] = {
> +   _dev_attr_in0_input.dev_attr.attr,
> +   NULL
> +};
> +
> +ATTRIBUTE_GROUPS(ltc2471);
> +
> +static int ltc2471_i2c_probe(struct i2c_client *i2c,
> +const struct i2c_device_id *id)
> +{
> +   int ret;
> +   struct device *hwmon_dev;
> +   struct ltc2471_data *data;
> +
> +   if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C))
> +   return -ENODEV;
> +
> +   data = devm_kzalloc(>dev, sizeof(struct ltc2471_data), 
> GFP_KERNEL);

sizeof(*data)

> +   if (unlikely(!data))
> +   return -ENOMEM;
> +
> +   data->i2c = i2c;
> +   data->differential = (id->driver_data == ltc2473);
> +
> +   /* Trigger once to start conversion and check if chip is there */
> +   ret = ltc2471_get_value(i2c);
> +   if (ret < 0) {
> +   

Re: [kernel-sched-cputime] question about probable bug in cputime_adjust()

[Frans Klaver]

On 29 June 2017 01:57:19 CEST, "Gustavo A. R. Silva"  
wrote:
 --- a/kernel/sched/cputime.c
 +++ b/kernel/sched/cputime.c
 @@ -637,9 +637,10 @@ static void cputime_adjust(struct task_cputime
>*curr,
  *= (rtime_i+1 - rtime_i) + utime_i
  *>= utime_i
  */
 -   if (stime < prev->stime)
 +   if (stime < prev->stime) {
 stime = prev->stime;
 -   utime = rtime - stime;
 +   utime = rtime - stime;
 +   }


 If you confirm this, I will send a patch in a full and proper form.

 I'd really appreciate your comments.
>>>
>>> If you do that, how would you meet the guarantee made in line 583?
>>>
>
>You are right, I see now.
>
>Then in this case the following patch would be the way to go:
>
>--- a/kernel/sched/cputime.c
>+++ b/kernel/sched/cputime.c
>@@ -615,10 +615,8 @@ static void cputime_adjust(struct task_cputime
>*curr,
>   * userspace. Once a task gets some ticks, the monotonicy code at
>  * 'update' will ensure things converge to the observed ratio.
>  */
>-   if (stime == 0) {
>-   utime = rtime;
>+   if (stime == 0)
> goto update;
>-   }
>
> if (utime == 0) {
> stime = rtime;
>
>
>but I think this one is even better:
>
>
>--- a/kernel/sched/cputime.c
>+++ b/kernel/sched/cputime.c
>@@ -615,19 +615,11 @@ static void cputime_adjust(struct task_cputime
>*curr,
>   * userspace. Once a task gets some ticks, the monotonicy code at
>  * 'update' will ensure things converge to the observed ratio.
>  */
>-   if (stime == 0) {
>-   utime = rtime;
>-   goto update;
>-   }
>-
>-   if (utime == 0) {
>+   if (stime != 0 && utime == 0)
> stime = rtime;
>-   goto update;
>-   }
>-
>-   stime = scale_stime(stime, rtime, stime + utime);
>+   else
>+   stime = scale_stime(stime, rtime, stime + utime);

I don't think it is better. The stime == 0 case is gone now. So scale_time() 
will be called in that case. This whole if/else block should only be executed 
if stime != 0. 

Re: [kernel-sched-cputime] question about probable bug in cputime_adjust()

[Frans Klaver]

On 29 June 2017 01:57:19 CEST, "Gustavo A. R. Silva"  
wrote:
 --- a/kernel/sched/cputime.c
 +++ b/kernel/sched/cputime.c
 @@ -637,9 +637,10 @@ static void cputime_adjust(struct task_cputime
>*curr,
  *= (rtime_i+1 - rtime_i) + utime_i
  *>= utime_i
  */
 -   if (stime < prev->stime)
 +   if (stime < prev->stime) {
 stime = prev->stime;
 -   utime = rtime - stime;
 +   utime = rtime - stime;
 +   }


 If you confirm this, I will send a patch in a full and proper form.

 I'd really appreciate your comments.
>>>
>>> If you do that, how would you meet the guarantee made in line 583?
>>>
>
>You are right, I see now.
>
>Then in this case the following patch would be the way to go:
>
>--- a/kernel/sched/cputime.c
>+++ b/kernel/sched/cputime.c
>@@ -615,10 +615,8 @@ static void cputime_adjust(struct task_cputime
>*curr,
>   * userspace. Once a task gets some ticks, the monotonicy code at
>  * 'update' will ensure things converge to the observed ratio.
>  */
>-   if (stime == 0) {
>-   utime = rtime;
>+   if (stime == 0)
> goto update;
>-   }
>
> if (utime == 0) {
> stime = rtime;
>
>
>but I think this one is even better:
>
>
>--- a/kernel/sched/cputime.c
>+++ b/kernel/sched/cputime.c
>@@ -615,19 +615,11 @@ static void cputime_adjust(struct task_cputime
>*curr,
>   * userspace. Once a task gets some ticks, the monotonicy code at
>  * 'update' will ensure things converge to the observed ratio.
>  */
>-   if (stime == 0) {
>-   utime = rtime;
>-   goto update;
>-   }
>-
>-   if (utime == 0) {
>+   if (stime != 0 && utime == 0)
> stime = rtime;
>-   goto update;
>-   }
>-
>-   stime = scale_stime(stime, rtime, stime + utime);
>+   else
>+   stime = scale_stime(stime, rtime, stime + utime);

I don't think it is better. The stime == 0 case is gone now. So scale_time() 
will be called in that case. This whole if/else block should only be executed 
if stime != 0. 

Re: [PATCH] scsi: hisi_sas: optimise DMA slot memory

[Frans Klaver]

On Wed, Jun 28, 2017 at 5:25 PM, John Garry  wrote:
> From: Xiaofei Tan 
>
> Currently we allocate 3 sets of DMA memories from separate
> pools for each slot. This is inefficient in terms of memory usage
> (buffers are less than 1 page in size, so we lose due to alignment),
> and also time spend in doing 3 allocations + de-allocations per slot,
> instead of 1.
>
> To optimise, combine the 3 DMA buffers into a single buffer from a
> single pool.
>
> Signed-off-by: John Garry 
> Signed-off-by: Xiaofei Tan 

I'm not sure how strictly this is done, but typically the author signs
off first.

Frans

Re: [PATCH] scsi: hisi_sas: optimise DMA slot memory

[Frans Klaver]

On Wed, Jun 28, 2017 at 5:25 PM, John Garry  wrote:
> From: Xiaofei Tan 
>
> Currently we allocate 3 sets of DMA memories from separate
> pools for each slot. This is inefficient in terms of memory usage
> (buffers are less than 1 page in size, so we lose due to alignment),
> and also time spend in doing 3 allocations + de-allocations per slot,
> instead of 1.
>
> To optimise, combine the 3 DMA buffers into a single buffer from a
> single pool.
>
> Signed-off-by: John Garry 
> Signed-off-by: Xiaofei Tan 

I'm not sure how strictly this is done, but typically the author signs
off first.

Frans

Re: [PATCH] staging: rts5208 : Fixing coding style warnings

[Frans Klaver]

Hi,

On Wed, Jun 28, 2017 at 2:50 PM, Simo Koskinen  wrote:
> Fixed following warnings found by checkpatch.pl script:
>
> WARNING: Prefer using '"%s...", __func__' to using '',
> this function's name, in a string
>
> Signed-off-by: Simo Koskinen 
> ---
>  drivers/staging/rts5208/rtsx.c  |  2 --
>  drivers/staging/rts5208/rtsx_chip.c |  4 ++--
>  drivers/staging/rts5208/sd.c|  8 
>  drivers/staging/rts5208/spi.c   |  8 
>  drivers/staging/rts5208/xd.c| 11 ++-
>  5 files changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
> index b8177f5..c7ae702 100644
> --- a/drivers/staging/rts5208/rtsx.c
> +++ b/drivers/staging/rts5208/rtsx.c
> @@ -1009,8 +1009,6 @@ static void rtsx_remove(struct pci_dev *pci)
>  {
> struct rtsx_dev *dev = pci_get_drvdata(pci);
>
> -   dev_info(>dev, "rtsx_remove() called\n");
> -
> quiesce_and_remove_host(dev);
> release_everything(dev);
>  }
> diff --git a/drivers/staging/rts5208/rtsx_chip.c 
> b/drivers/staging/rts5208/rtsx_chip.c
> index 7f4107b..4ad472d 100644
> --- a/drivers/staging/rts5208/rtsx_chip.c
> +++ b/drivers/staging/rts5208/rtsx_chip.c
> @@ -616,8 +616,8 @@ int rtsx_reset_chip(struct rtsx_chip *chip)
> else
> retval = rtsx_pre_handle_sdio_new(chip);
>
> -   dev_dbg(rtsx_dev(chip), "chip->need_reset = 0x%x 
> (rtsx_reset_chip)\n",
> -   (unsigned int)(chip->need_reset));
> +   dev_dbg(rtsx_dev(chip), "chip->need_reset = 0x%x (%s)\n",
> +   (unsigned int)(chip->need_reset), __func__);
>  #else  /* HW_AUTO_SWITCH_SD_BUS */
> retval = rtsx_pre_handle_sdio_old(chip);
>  #endif  /* HW_AUTO_SWITCH_SD_BUS */
> diff --git a/drivers/staging/rts5208/sd.c b/drivers/staging/rts5208/sd.c
> index c2eb072..ae774ff 100644
> --- a/drivers/staging/rts5208/sd.c
> +++ b/drivers/staging/rts5208/sd.c
> @@ -910,8 +910,8 @@ static int sd_change_phase(struct rtsx_chip *chip, u8 
> sample_point, u8 tune_dir)
> int retval;
> bool ddr_rx = false;
>
> -   dev_dbg(rtsx_dev(chip), "sd_change_phase (sample_point = %d, tune_dir 
> = %d)\n",
> -   sample_point, tune_dir);
> +   dev_dbg(rtsx_dev(chip), "%s (sample_point = %d, tune_dir = %d)\n",
> +   __func__, sample_point, tune_dir);
>
> if (tune_dir == TUNE_RX) {
> SD_VP_CTL = SD_VPRX_CTL;
> @@ -3575,8 +3575,8 @@ static int reset_mmc_only(struct rtsx_chip *chip)
> return STATUS_FAIL;
> }
>
> -   dev_dbg(rtsx_dev(chip), "In reset_mmc_only, sd_card->sd_type = 
> 0x%x\n",
> -   sd_card->sd_type);
> +   dev_dbg(rtsx_dev(chip), "In %s, sd_card->sd_type = 0x%x\n",
> +   __func__, sd_card->sd_type);
>
> return STATUS_SUCCESS;
>  }
> diff --git a/drivers/staging/rts5208/spi.c b/drivers/staging/rts5208/spi.c
> index 8b8cd95..feb9c2b 100644
> --- a/drivers/staging/rts5208/spi.c
> +++ b/drivers/staging/rts5208/spi.c
> @@ -520,8 +520,8 @@ int spi_get_status(struct scsi_cmnd *srb, struct 
> rtsx_chip *chip)
>  {
> struct spi_info *spi = >spi;
>
> -   dev_dbg(rtsx_dev(chip), "spi_get_status: err_code = 0x%x\n",
> -   spi->err_code);
> +   dev_dbg(rtsx_dev(chip), "%s: err_code = 0x%x\n",
> +   __func__, spi->err_code);
> rtsx_stor_set_xfer_buf(>err_code,
>min_t(int, scsi_bufflen(srb), 1), srb);
> scsi_set_resid(srb, scsi_bufflen(srb) - 1);
> @@ -543,8 +543,8 @@ int spi_set_parameter(struct scsi_cmnd *srb, struct 
> rtsx_chip *chip)
> spi->clk_div = ((u16)(srb->cmnd[4]) << 8) | srb->cmnd[5];
> spi->write_en = srb->cmnd[6];
>
> -   dev_dbg(rtsx_dev(chip), "spi_set_parameter: spi_clock = %d, clk_div = 
> %d, write_en = %d\n",
> -   spi->spi_clock, spi->clk_div, spi->write_en);
> +   dev_dbg(rtsx_dev(chip), "%s: spi_clock = %d, clk_div = %d, write_en = 
> %d\n",
> +   __func__, spi->spi_clock, spi->clk_div, spi->write_en);
>
> return STATUS_SUCCESS;
>  }
> diff --git a/drivers/staging/rts5208/xd.c b/drivers/staging/rts5208/xd.c
> index 74d36f9..dc0baf0 100644
> --- a/drivers/staging/rts5208/xd.c
> +++ b/drivers/staging/rts5208/xd.c
> @@ -885,8 +885,8 @@ static int xd_init_l2p_tbl(struct rtsx_chip *chip)
> struct xd_info *xd_card = >xd_card;
> int size, i;
>
> -   dev_dbg(rtsx_dev(chip), "xd_init_l2p_tbl: zone_cnt = %d\n",
> -   xd_card->zone_cnt);
> +   dev_dbg(rtsx_dev(chip), "%s: zone_cnt = %d\n",
> +   __func__, xd_card->zone_cnt);
>
> if (xd_card->zone_cnt < 1) {
> rtsx_trace(chip);
> @@ -1026,7 +1026,8 @@ static u32 xd_get_l2p_tbl(struct rtsx_chip *chip, int 
> zone_no, u16 log_off)
>  #ifdef XD_DELAY_WRITE
>   

Re: [PATCH] staging: rts5208 : Fixing coding style warnings

[Frans Klaver]

Hi,

On Wed, Jun 28, 2017 at 2:50 PM, Simo Koskinen  wrote:
> Fixed following warnings found by checkpatch.pl script:
>
> WARNING: Prefer using '"%s...", __func__' to using '',
> this function's name, in a string
>
> Signed-off-by: Simo Koskinen 
> ---
>  drivers/staging/rts5208/rtsx.c  |  2 --
>  drivers/staging/rts5208/rtsx_chip.c |  4 ++--
>  drivers/staging/rts5208/sd.c|  8 
>  drivers/staging/rts5208/spi.c   |  8 
>  drivers/staging/rts5208/xd.c| 11 ++-
>  5 files changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
> index b8177f5..c7ae702 100644
> --- a/drivers/staging/rts5208/rtsx.c
> +++ b/drivers/staging/rts5208/rtsx.c
> @@ -1009,8 +1009,6 @@ static void rtsx_remove(struct pci_dev *pci)
>  {
> struct rtsx_dev *dev = pci_get_drvdata(pci);
>
> -   dev_info(>dev, "rtsx_remove() called\n");
> -
> quiesce_and_remove_host(dev);
> release_everything(dev);
>  }
> diff --git a/drivers/staging/rts5208/rtsx_chip.c 
> b/drivers/staging/rts5208/rtsx_chip.c
> index 7f4107b..4ad472d 100644
> --- a/drivers/staging/rts5208/rtsx_chip.c
> +++ b/drivers/staging/rts5208/rtsx_chip.c
> @@ -616,8 +616,8 @@ int rtsx_reset_chip(struct rtsx_chip *chip)
> else
> retval = rtsx_pre_handle_sdio_new(chip);
>
> -   dev_dbg(rtsx_dev(chip), "chip->need_reset = 0x%x 
> (rtsx_reset_chip)\n",
> -   (unsigned int)(chip->need_reset));
> +   dev_dbg(rtsx_dev(chip), "chip->need_reset = 0x%x (%s)\n",
> +   (unsigned int)(chip->need_reset), __func__);
>  #else  /* HW_AUTO_SWITCH_SD_BUS */
> retval = rtsx_pre_handle_sdio_old(chip);
>  #endif  /* HW_AUTO_SWITCH_SD_BUS */
> diff --git a/drivers/staging/rts5208/sd.c b/drivers/staging/rts5208/sd.c
> index c2eb072..ae774ff 100644
> --- a/drivers/staging/rts5208/sd.c
> +++ b/drivers/staging/rts5208/sd.c
> @@ -910,8 +910,8 @@ static int sd_change_phase(struct rtsx_chip *chip, u8 
> sample_point, u8 tune_dir)
> int retval;
> bool ddr_rx = false;
>
> -   dev_dbg(rtsx_dev(chip), "sd_change_phase (sample_point = %d, tune_dir 
> = %d)\n",
> -   sample_point, tune_dir);
> +   dev_dbg(rtsx_dev(chip), "%s (sample_point = %d, tune_dir = %d)\n",
> +   __func__, sample_point, tune_dir);
>
> if (tune_dir == TUNE_RX) {
> SD_VP_CTL = SD_VPRX_CTL;
> @@ -3575,8 +3575,8 @@ static int reset_mmc_only(struct rtsx_chip *chip)
> return STATUS_FAIL;
> }
>
> -   dev_dbg(rtsx_dev(chip), "In reset_mmc_only, sd_card->sd_type = 
> 0x%x\n",
> -   sd_card->sd_type);
> +   dev_dbg(rtsx_dev(chip), "In %s, sd_card->sd_type = 0x%x\n",
> +   __func__, sd_card->sd_type);
>
> return STATUS_SUCCESS;
>  }
> diff --git a/drivers/staging/rts5208/spi.c b/drivers/staging/rts5208/spi.c
> index 8b8cd95..feb9c2b 100644
> --- a/drivers/staging/rts5208/spi.c
> +++ b/drivers/staging/rts5208/spi.c
> @@ -520,8 +520,8 @@ int spi_get_status(struct scsi_cmnd *srb, struct 
> rtsx_chip *chip)
>  {
> struct spi_info *spi = >spi;
>
> -   dev_dbg(rtsx_dev(chip), "spi_get_status: err_code = 0x%x\n",
> -   spi->err_code);
> +   dev_dbg(rtsx_dev(chip), "%s: err_code = 0x%x\n",
> +   __func__, spi->err_code);
> rtsx_stor_set_xfer_buf(>err_code,
>min_t(int, scsi_bufflen(srb), 1), srb);
> scsi_set_resid(srb, scsi_bufflen(srb) - 1);
> @@ -543,8 +543,8 @@ int spi_set_parameter(struct scsi_cmnd *srb, struct 
> rtsx_chip *chip)
> spi->clk_div = ((u16)(srb->cmnd[4]) << 8) | srb->cmnd[5];
> spi->write_en = srb->cmnd[6];
>
> -   dev_dbg(rtsx_dev(chip), "spi_set_parameter: spi_clock = %d, clk_div = 
> %d, write_en = %d\n",
> -   spi->spi_clock, spi->clk_div, spi->write_en);
> +   dev_dbg(rtsx_dev(chip), "%s: spi_clock = %d, clk_div = %d, write_en = 
> %d\n",
> +   __func__, spi->spi_clock, spi->clk_div, spi->write_en);
>
> return STATUS_SUCCESS;
>  }
> diff --git a/drivers/staging/rts5208/xd.c b/drivers/staging/rts5208/xd.c
> index 74d36f9..dc0baf0 100644
> --- a/drivers/staging/rts5208/xd.c
> +++ b/drivers/staging/rts5208/xd.c
> @@ -885,8 +885,8 @@ static int xd_init_l2p_tbl(struct rtsx_chip *chip)
> struct xd_info *xd_card = >xd_card;
> int size, i;
>
> -   dev_dbg(rtsx_dev(chip), "xd_init_l2p_tbl: zone_cnt = %d\n",
> -   xd_card->zone_cnt);
> +   dev_dbg(rtsx_dev(chip), "%s: zone_cnt = %d\n",
> +   __func__, xd_card->zone_cnt);
>
> if (xd_card->zone_cnt < 1) {
> rtsx_trace(chip);
> @@ -1026,7 +1026,8 @@ static u32 xd_get_l2p_tbl(struct rtsx_chip *chip, int 
> zone_no, u16 log_off)
>  #ifdef XD_DELAY_WRITE
> retval = xd_delay_write(chip);

Re: [PATCH] Staging: unisys: visorbus: Fix coding style warning from checkpatch.pl.

[Frans Klaver]

Hi,

On Wed, Jun 28, 2017 at 4:49 AM, Quytelda Kahja  wrote:
> Replace the literal function name "visorbus_create_instance" with the format
> specifier "%s" so it can be dynamically filled by the __func__ macro.

On a general note, I think the actual change or effect is more import
to mention in the subject than the fact that you "fix some issue
highlighted by checkpatch".

Something like

Subject: [PATCH] staging: unisys: visorbus: use __func__ instead of
function name

Of course you can still mention that checkpatch highlighted the issue
for you in the description.

>
> Signed-off-by: Quytelda Kahja 
> ---
>  drivers/staging/unisys/visorbus/visorbus_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c 
> b/drivers/staging/unisys/visorbus/visorbus_main.c
> index 1c785dd19ddd..1c6dc3a3e64a 100644
> --- a/drivers/staging/unisys/visorbus/visorbus_main.c
> +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
> @@ -1030,7 +1030,7 @@ visorbus_create_instance(struct visor_device *dev)
>  err_debugfs_dir:
> debugfs_remove_recursive(dev->debugfs_dir);
> kfree(hdr_info);
> -   dev_err(>device, "visorbus_create_instance failed: %d\n", err);
> +   dev_err(>device, "%s failed: %d\n", __func__, err);
> return err;
>  }

It looks sane, anyway.

Frans

Re: [PATCH] Staging: unisys: visorbus: Fix coding style warning from checkpatch.pl.

[Frans Klaver]

Hi,

On Wed, Jun 28, 2017 at 4:49 AM, Quytelda Kahja  wrote:
> Replace the literal function name "visorbus_create_instance" with the format
> specifier "%s" so it can be dynamically filled by the __func__ macro.

On a general note, I think the actual change or effect is more import
to mention in the subject than the fact that you "fix some issue
highlighted by checkpatch".

Something like

Subject: [PATCH] staging: unisys: visorbus: use __func__ instead of
function name

Of course you can still mention that checkpatch highlighted the issue
for you in the description.

>
> Signed-off-by: Quytelda Kahja 
> ---
>  drivers/staging/unisys/visorbus/visorbus_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c 
> b/drivers/staging/unisys/visorbus/visorbus_main.c
> index 1c785dd19ddd..1c6dc3a3e64a 100644
> --- a/drivers/staging/unisys/visorbus/visorbus_main.c
> +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
> @@ -1030,7 +1030,7 @@ visorbus_create_instance(struct visor_device *dev)
>  err_debugfs_dir:
> debugfs_remove_recursive(dev->debugfs_dir);
> kfree(hdr_info);
> -   dev_err(>device, "visorbus_create_instance failed: %d\n", err);
> +   dev_err(>device, "%s failed: %d\n", __func__, err);
> return err;
>  }

It looks sane, anyway.

Frans

Re: [kernel-sched-cputime] question about probable bug in cputime_adjust()

[Frans Klaver]

On Wed, Jun 28, 2017 at 7:35 AM, Frans Klaver <franskla...@gmail.com> wrote:
> On Wed, Jun 28, 2017 at 1:03 AM, Gustavo A. R. Silva
> <garsi...@embeddedor.com> wrote:
>>
>> Hello everybody,
>>
>> While looking into Coverity ID 1371643 I ran into the following piece of
>> code at kernel/sched/cputime.c:568:
>>
>> 568/*
>> 569 * Adjust tick based cputime random precision against scheduler runtime
>> 570 * accounting.
>> 571 *
>> 572 * Tick based cputime accounting depend on random scheduling timeslices
>> of a
>> 573 * task to be interrupted or not by the timer.  Depending on these
>> 574 * circumstances, the number of these interrupts may be over or
>> 575 * under-optimistic, matching the real user and system cputime with a
>> variable
>> 576 * precision.
>> 577 *
>> 578 * Fix this by scaling these tick based values against the total runtime
>> 579 * accounted by the CFS scheduler.
>> 580 *
>> 581 * This code provides the following guarantees:
>> 582 *
>> 583 *   stime + utime == rtime
>> 584 *   stime_i+1 >= stime_i, utime_i+1 >= utime_i
>> 585 *
>> 586 * Assuming that rtime_i+1 >= rtime_i.
>> 587 */
>> 588static void cputime_adjust(struct task_cputime *curr,
>> 589   struct prev_cputime *prev,
>> 590   u64 *ut, u64 *st)
>> 591{
>> 592u64 rtime, stime, utime;
>> 593unsigned long flags;
>> 594
>> 595/* Serialize concurrent callers such that we can honour our
>> guarantees */
>> 596raw_spin_lock_irqsave(>lock, flags);
>> 597rtime = curr->sum_exec_runtime;
>> 598
>> 599/*
>> 600 * This is possible under two circumstances:
>> 601 *  - rtime isn't monotonic after all (a bug);
>> 602 *  - we got reordered by the lock.
>> 603 *
>> 604 * In both cases this acts as a filter such that the rest of the
>> code
>> 605 * can assume it is monotonic regardless of anything else.
>> 606 */
>> 607if (prev->stime + prev->utime >= rtime)
>> 608goto out;
>> 609
>> 610stime = curr->stime;
>> 611utime = curr->utime;
>> 612
>> 613/*
>> 614 * If either stime or both stime and utime are 0, assume all
>> runtime is
>> 615 * userspace. Once a task gets some ticks, the monotonicy code at
>> 616 * 'update' will ensure things converge to the observed ratio.
>> 617 */
>> 618if (stime == 0) {
>> 619utime = rtime;
>> 620goto update;
>> 621}
>> 622
>> 623if (utime == 0) {
>> 624stime = rtime;
>> 625goto update;
>> 626}
>> 627
>> 628stime = scale_stime(stime, rtime, stime + utime);
>> 629
>> 630update:
>> 631/*
>> 632 * Make sure stime doesn't go backwards; this preserves
>> monotonicity
>> 633 * for utime because rtime is monotonic.
>> 634 *
>> 635 *  utime_i+1 = rtime_i+1 - stime_i
>> 636 *= rtime_i+1 - (rtime_i - utime_i)
>> 637 *= (rtime_i+1 - rtime_i) + utime_i
>> 638 *>= utime_i
>> 639 */
>> 640if (stime < prev->stime)
>> 641stime = prev->stime;
>> 642utime = rtime - stime;
>> 643
>> 644/*
>> 645 * Make sure utime doesn't go backwards; this still preserves
>> 646 * monotonicity for stime, analogous argument to above.
>> 647 */
>> 648if (utime < prev->utime) {
>> 649utime = prev->utime;
>> 650stime = rtime - utime;
>> 651}
>> 652
>> 653prev->stime = stime;
>> 654prev->utime = utime;
>> 655out:
>> 656*ut = prev->utime;
>> 657*st = prev->stime;
>> 658raw_spin_unlock_irqrestore(>lock, flags);
>> 659}
>>
>>
>> The issue here is that the value assigned to variable utime at line 619 is
>> overwritten at line 642, which would make such variable assignment useless.
>
> It isn't completely useless, since utime is used in line 628 to calculate 
> stime.

Oh, I missed 'goto update'. Never mind about this one.


>> But I'm suspicious that such assignment is actually correct and that line
>> 642 should be included into the IF block at line 640. Something similar to
>> the following patch:
>>
>> --- a/kernel/sched/cputime.c
>> +++ b/kernel/sched/cputime.c
>> @@ -637,9 +637,10 @@ static void cputime_adjust(struct task_cputime *curr,
>>  *= (rtime_i+1 - rtime_i) + utime_i
>>  *>= utime_i
>>  */
>> -   if (stime < prev->stime)
>> +   if (stime < prev->stime) {
>> stime = prev->stime;
>> -   utime = rtime - stime;
>> +   utime = rtime - stime;
>> +   }
>>
>>
>> If you confirm this, I will send a patch in a full and proper form.
>>
>> I'd really appreciate your comments.
>
> If you do that, how would you meet the guarantee made in line 583?
>
> Frans

Re: [kernel-sched-cputime] question about probable bug in cputime_adjust()

[Frans Klaver]

On Wed, Jun 28, 2017 at 7:35 AM, Frans Klaver  wrote:
> On Wed, Jun 28, 2017 at 1:03 AM, Gustavo A. R. Silva
>  wrote:
>>
>> Hello everybody,
>>
>> While looking into Coverity ID 1371643 I ran into the following piece of
>> code at kernel/sched/cputime.c:568:
>>
>> 568/*
>> 569 * Adjust tick based cputime random precision against scheduler runtime
>> 570 * accounting.
>> 571 *
>> 572 * Tick based cputime accounting depend on random scheduling timeslices
>> of a
>> 573 * task to be interrupted or not by the timer.  Depending on these
>> 574 * circumstances, the number of these interrupts may be over or
>> 575 * under-optimistic, matching the real user and system cputime with a
>> variable
>> 576 * precision.
>> 577 *
>> 578 * Fix this by scaling these tick based values against the total runtime
>> 579 * accounted by the CFS scheduler.
>> 580 *
>> 581 * This code provides the following guarantees:
>> 582 *
>> 583 *   stime + utime == rtime
>> 584 *   stime_i+1 >= stime_i, utime_i+1 >= utime_i
>> 585 *
>> 586 * Assuming that rtime_i+1 >= rtime_i.
>> 587 */
>> 588static void cputime_adjust(struct task_cputime *curr,
>> 589   struct prev_cputime *prev,
>> 590   u64 *ut, u64 *st)
>> 591{
>> 592u64 rtime, stime, utime;
>> 593unsigned long flags;
>> 594
>> 595/* Serialize concurrent callers such that we can honour our
>> guarantees */
>> 596raw_spin_lock_irqsave(>lock, flags);
>> 597rtime = curr->sum_exec_runtime;
>> 598
>> 599/*
>> 600 * This is possible under two circumstances:
>> 601 *  - rtime isn't monotonic after all (a bug);
>> 602 *  - we got reordered by the lock.
>> 603 *
>> 604 * In both cases this acts as a filter such that the rest of the
>> code
>> 605 * can assume it is monotonic regardless of anything else.
>> 606 */
>> 607if (prev->stime + prev->utime >= rtime)
>> 608goto out;
>> 609
>> 610stime = curr->stime;
>> 611utime = curr->utime;
>> 612
>> 613/*
>> 614 * If either stime or both stime and utime are 0, assume all
>> runtime is
>> 615 * userspace. Once a task gets some ticks, the monotonicy code at
>> 616 * 'update' will ensure things converge to the observed ratio.
>> 617 */
>> 618if (stime == 0) {
>> 619utime = rtime;
>> 620goto update;
>> 621}
>> 622
>> 623if (utime == 0) {
>> 624stime = rtime;
>> 625goto update;
>> 626}
>> 627
>> 628stime = scale_stime(stime, rtime, stime + utime);
>> 629
>> 630update:
>> 631/*
>> 632 * Make sure stime doesn't go backwards; this preserves
>> monotonicity
>> 633 * for utime because rtime is monotonic.
>> 634 *
>> 635 *  utime_i+1 = rtime_i+1 - stime_i
>> 636 *= rtime_i+1 - (rtime_i - utime_i)
>> 637 *= (rtime_i+1 - rtime_i) + utime_i
>> 638 *>= utime_i
>> 639 */
>> 640if (stime < prev->stime)
>> 641stime = prev->stime;
>> 642utime = rtime - stime;
>> 643
>> 644/*
>> 645 * Make sure utime doesn't go backwards; this still preserves
>> 646 * monotonicity for stime, analogous argument to above.
>> 647 */
>> 648if (utime < prev->utime) {
>> 649utime = prev->utime;
>> 650stime = rtime - utime;
>> 651}
>> 652
>> 653prev->stime = stime;
>> 654prev->utime = utime;
>> 655out:
>> 656*ut = prev->utime;
>> 657*st = prev->stime;
>> 658raw_spin_unlock_irqrestore(>lock, flags);
>> 659}
>>
>>
>> The issue here is that the value assigned to variable utime at line 619 is
>> overwritten at line 642, which would make such variable assignment useless.
>
> It isn't completely useless, since utime is used in line 628 to calculate 
> stime.

Oh, I missed 'goto update'. Never mind about this one.


>> But I'm suspicious that such assignment is actually correct and that line
>> 642 should be included into the IF block at line 640. Something similar to
>> the following patch:
>>
>> --- a/kernel/sched/cputime.c
>> +++ b/kernel/sched/cputime.c
>> @@ -637,9 +637,10 @@ static void cputime_adjust(struct task_cputime *curr,
>>  *= (rtime_i+1 - rtime_i) + utime_i
>>  *>= utime_i
>>  */
>> -   if (stime < prev->stime)
>> +   if (stime < prev->stime) {
>> stime = prev->stime;
>> -   utime = rtime - stime;
>> +   utime = rtime - stime;
>> +   }
>>
>>
>> If you confirm this, I will send a patch in a full and proper form.
>>
>> I'd really appreciate your comments.
>
> If you do that, how would you meet the guarantee made in line 583?
>
> Frans

Re: [kernel-sched-cputime] question about probable bug in cputime_adjust()

[Frans Klaver]

On Wed, Jun 28, 2017 at 1:03 AM, Gustavo A. R. Silva
 wrote:
>
> Hello everybody,
>
> While looking into Coverity ID 1371643 I ran into the following piece of
> code at kernel/sched/cputime.c:568:
>
> 568/*
> 569 * Adjust tick based cputime random precision against scheduler runtime
> 570 * accounting.
> 571 *
> 572 * Tick based cputime accounting depend on random scheduling timeslices
> of a
> 573 * task to be interrupted or not by the timer.  Depending on these
> 574 * circumstances, the number of these interrupts may be over or
> 575 * under-optimistic, matching the real user and system cputime with a
> variable
> 576 * precision.
> 577 *
> 578 * Fix this by scaling these tick based values against the total runtime
> 579 * accounted by the CFS scheduler.
> 580 *
> 581 * This code provides the following guarantees:
> 582 *
> 583 *   stime + utime == rtime
> 584 *   stime_i+1 >= stime_i, utime_i+1 >= utime_i
> 585 *
> 586 * Assuming that rtime_i+1 >= rtime_i.
> 587 */
> 588static void cputime_adjust(struct task_cputime *curr,
> 589   struct prev_cputime *prev,
> 590   u64 *ut, u64 *st)
> 591{
> 592u64 rtime, stime, utime;
> 593unsigned long flags;
> 594
> 595/* Serialize concurrent callers such that we can honour our
> guarantees */
> 596raw_spin_lock_irqsave(>lock, flags);
> 597rtime = curr->sum_exec_runtime;
> 598
> 599/*
> 600 * This is possible under two circumstances:
> 601 *  - rtime isn't monotonic after all (a bug);
> 602 *  - we got reordered by the lock.
> 603 *
> 604 * In both cases this acts as a filter such that the rest of the
> code
> 605 * can assume it is monotonic regardless of anything else.
> 606 */
> 607if (prev->stime + prev->utime >= rtime)
> 608goto out;
> 609
> 610stime = curr->stime;
> 611utime = curr->utime;
> 612
> 613/*
> 614 * If either stime or both stime and utime are 0, assume all
> runtime is
> 615 * userspace. Once a task gets some ticks, the monotonicy code at
> 616 * 'update' will ensure things converge to the observed ratio.
> 617 */
> 618if (stime == 0) {
> 619utime = rtime;
> 620goto update;
> 621}
> 622
> 623if (utime == 0) {
> 624stime = rtime;
> 625goto update;
> 626}
> 627
> 628stime = scale_stime(stime, rtime, stime + utime);
> 629
> 630update:
> 631/*
> 632 * Make sure stime doesn't go backwards; this preserves
> monotonicity
> 633 * for utime because rtime is monotonic.
> 634 *
> 635 *  utime_i+1 = rtime_i+1 - stime_i
> 636 *= rtime_i+1 - (rtime_i - utime_i)
> 637 *= (rtime_i+1 - rtime_i) + utime_i
> 638 *>= utime_i
> 639 */
> 640if (stime < prev->stime)
> 641stime = prev->stime;
> 642utime = rtime - stime;
> 643
> 644/*
> 645 * Make sure utime doesn't go backwards; this still preserves
> 646 * monotonicity for stime, analogous argument to above.
> 647 */
> 648if (utime < prev->utime) {
> 649utime = prev->utime;
> 650stime = rtime - utime;
> 651}
> 652
> 653prev->stime = stime;
> 654prev->utime = utime;
> 655out:
> 656*ut = prev->utime;
> 657*st = prev->stime;
> 658raw_spin_unlock_irqrestore(>lock, flags);
> 659}
>
>
> The issue here is that the value assigned to variable utime at line 619 is
> overwritten at line 642, which would make such variable assignment useless.

It isn't completely useless, since utime is used in line 628 to calculate stime.


> But I'm suspicious that such assignment is actually correct and that line
> 642 should be included into the IF block at line 640. Something similar to
> the following patch:
>
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -637,9 +637,10 @@ static void cputime_adjust(struct task_cputime *curr,
>  *= (rtime_i+1 - rtime_i) + utime_i
>  *>= utime_i
>  */
> -   if (stime < prev->stime)
> +   if (stime < prev->stime) {
> stime = prev->stime;
> -   utime = rtime - stime;
> +   utime = rtime - stime;
> +   }
>
>
> If you confirm this, I will send a patch in a full and proper form.
>
> I'd really appreciate your comments.

If you do that, how would you meet the guarantee made in line 583?

Frans

Re: [kernel-sched-cputime] question about probable bug in cputime_adjust()

[Frans Klaver]

On Wed, Jun 28, 2017 at 1:03 AM, Gustavo A. R. Silva
 wrote:
>
> Hello everybody,
>
> While looking into Coverity ID 1371643 I ran into the following piece of
> code at kernel/sched/cputime.c:568:
>
> 568/*
> 569 * Adjust tick based cputime random precision against scheduler runtime
> 570 * accounting.
> 571 *
> 572 * Tick based cputime accounting depend on random scheduling timeslices
> of a
> 573 * task to be interrupted or not by the timer.  Depending on these
> 574 * circumstances, the number of these interrupts may be over or
> 575 * under-optimistic, matching the real user and system cputime with a
> variable
> 576 * precision.
> 577 *
> 578 * Fix this by scaling these tick based values against the total runtime
> 579 * accounted by the CFS scheduler.
> 580 *
> 581 * This code provides the following guarantees:
> 582 *
> 583 *   stime + utime == rtime
> 584 *   stime_i+1 >= stime_i, utime_i+1 >= utime_i
> 585 *
> 586 * Assuming that rtime_i+1 >= rtime_i.
> 587 */
> 588static void cputime_adjust(struct task_cputime *curr,
> 589   struct prev_cputime *prev,
> 590   u64 *ut, u64 *st)
> 591{
> 592u64 rtime, stime, utime;
> 593unsigned long flags;
> 594
> 595/* Serialize concurrent callers such that we can honour our
> guarantees */
> 596raw_spin_lock_irqsave(>lock, flags);
> 597rtime = curr->sum_exec_runtime;
> 598
> 599/*
> 600 * This is possible under two circumstances:
> 601 *  - rtime isn't monotonic after all (a bug);
> 602 *  - we got reordered by the lock.
> 603 *
> 604 * In both cases this acts as a filter such that the rest of the
> code
> 605 * can assume it is monotonic regardless of anything else.
> 606 */
> 607if (prev->stime + prev->utime >= rtime)
> 608goto out;
> 609
> 610stime = curr->stime;
> 611utime = curr->utime;
> 612
> 613/*
> 614 * If either stime or both stime and utime are 0, assume all
> runtime is
> 615 * userspace. Once a task gets some ticks, the monotonicy code at
> 616 * 'update' will ensure things converge to the observed ratio.
> 617 */
> 618if (stime == 0) {
> 619utime = rtime;
> 620goto update;
> 621}
> 622
> 623if (utime == 0) {
> 624stime = rtime;
> 625goto update;
> 626}
> 627
> 628stime = scale_stime(stime, rtime, stime + utime);
> 629
> 630update:
> 631/*
> 632 * Make sure stime doesn't go backwards; this preserves
> monotonicity
> 633 * for utime because rtime is monotonic.
> 634 *
> 635 *  utime_i+1 = rtime_i+1 - stime_i
> 636 *= rtime_i+1 - (rtime_i - utime_i)
> 637 *= (rtime_i+1 - rtime_i) + utime_i
> 638 *>= utime_i
> 639 */
> 640if (stime < prev->stime)
> 641stime = prev->stime;
> 642utime = rtime - stime;
> 643
> 644/*
> 645 * Make sure utime doesn't go backwards; this still preserves
> 646 * monotonicity for stime, analogous argument to above.
> 647 */
> 648if (utime < prev->utime) {
> 649utime = prev->utime;
> 650stime = rtime - utime;
> 651}
> 652
> 653prev->stime = stime;
> 654prev->utime = utime;
> 655out:
> 656*ut = prev->utime;
> 657*st = prev->stime;
> 658raw_spin_unlock_irqrestore(>lock, flags);
> 659}
>
>
> The issue here is that the value assigned to variable utime at line 619 is
> overwritten at line 642, which would make such variable assignment useless.

It isn't completely useless, since utime is used in line 628 to calculate stime.


> But I'm suspicious that such assignment is actually correct and that line
> 642 should be included into the IF block at line 640. Something similar to
> the following patch:
>
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -637,9 +637,10 @@ static void cputime_adjust(struct task_cputime *curr,
>  *= (rtime_i+1 - rtime_i) + utime_i
>  *>= utime_i
>  */
> -   if (stime < prev->stime)
> +   if (stime < prev->stime) {
> stime = prev->stime;
> -   utime = rtime - stime;
> +   utime = rtime - stime;
> +   }
>
>
> If you confirm this, I will send a patch in a full and proper form.
>
> I'd really appreciate your comments.

If you do that, how would you meet the guarantee made in line 583?

Frans

Re: [PATCH V3] lightnvm: if LUNs are already allocated fix return

[Frans Klaver]

On Tue, Jun 27, 2017 at 1:55 PM, Rakesh Pandit <rak...@tuxera.com> wrote:
> While creating new device with NVM_DEV_CREATE if LUNs are already
> allocated ioctl would return -ENOMEM which is wrong.  This patch
> propagates -EBUSY from nvm_reserve_luns which is correct response.
>
> Fixes: ade69e243 ("lightnvm: merge gennvm with core")
> Signed-off-by: Rakesh Pandit <rak...@tuxera.com>

Reviewed-by: Frans Klaver <franskla...@gmail.com>

> ---
>
> V3: Propagate return value from nvm_reserve_luns instead of hard-coding 
> (Frans)
> V2: return error code directly instead of using ret variable (Frans)
>
>  drivers/lightnvm/core.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index b8f82f5..ddae430 100644
> --- a/drivers/lightnvm/core.c
> +++ b/drivers/lightnvm/core.c
> @@ -252,8 +252,9 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct 
> nvm_ioctl_create *create)
> }
> mutex_unlock(>mlock);
>
> -   if (nvm_reserve_luns(dev, s->lun_begin, s->lun_end))
> -   return -ENOMEM;
> +   ret = nvm_reserve_luns(dev, s->lun_begin, s->lun_end);
> +   if (ret)
> +   return ret;
>
> t = kmalloc(sizeof(struct nvm_target), GFP_KERNEL);
> if (!t) {
> --
> 2.5.5
>

Re: [PATCH V3] lightnvm: if LUNs are already allocated fix return

[Frans Klaver]

On Tue, Jun 27, 2017 at 1:55 PM, Rakesh Pandit  wrote:
> While creating new device with NVM_DEV_CREATE if LUNs are already
> allocated ioctl would return -ENOMEM which is wrong.  This patch
> propagates -EBUSY from nvm_reserve_luns which is correct response.
>
> Fixes: ade69e243 ("lightnvm: merge gennvm with core")
> Signed-off-by: Rakesh Pandit 

Reviewed-by: Frans Klaver 

> ---
>
> V3: Propagate return value from nvm_reserve_luns instead of hard-coding 
> (Frans)
> V2: return error code directly instead of using ret variable (Frans)
>
>  drivers/lightnvm/core.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index b8f82f5..ddae430 100644
> --- a/drivers/lightnvm/core.c
> +++ b/drivers/lightnvm/core.c
> @@ -252,8 +252,9 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct 
> nvm_ioctl_create *create)
> }
> mutex_unlock(>mlock);
>
> -   if (nvm_reserve_luns(dev, s->lun_begin, s->lun_end))
> -   return -ENOMEM;
> +   ret = nvm_reserve_luns(dev, s->lun_begin, s->lun_end);
> +   if (ret)
> +   return ret;
>
> t = kmalloc(sizeof(struct nvm_target), GFP_KERNEL);
> if (!t) {
> --
> 2.5.5
>

Re: [PATCH V2] lightnvm: if LUNs are already allocated fix return

[Frans Klaver]

On Tue, Jun 27, 2017 at 1:23 PM, Rakesh Pandit <rak...@tuxera.com> wrote:
> On Tue, Jun 27, 2017 at 01:01:22PM +0200, Frans Klaver wrote:
>> On Tue, Jun 27, 2017 at 12:43 PM, Rakesh Pandit <rak...@tuxera.com> wrote:
>> > While creating new device with NVM_DEV_CREATE if LUNs are already
>> > allocated ioctl would return -ENOMEM which is wrong.  This patch
>> > propagates -EBUSY from nvm_reserve_luns which is correct response.
>> >
>> > Fixes: ade69e243 ("lightnvm: merge gennvm with core")
>> > Signed-off-by: Rakesh Pandit <rak...@tuxera.com>
>> > ---
>> >
>> > V2: return error code directly instead of using ret variable (Frans)
>> >
>> >  drivers/lightnvm/core.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
>> > index b8f82f5..c5d71c6 100644
>> > --- a/drivers/lightnvm/core.c
>> > +++ b/drivers/lightnvm/core.c
>> > @@ -253,7 +253,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct 
>> > nvm_ioctl_create *create)
>> > mutex_unlock(>mlock);
>> >
>> > if (nvm_reserve_luns(dev, s->lun_begin, s->lun_end))
>> > -   return -ENOMEM;
>> > +   return -EBUSY;
>>
>> Why aren't you propagating ret in this version?
>
> Well nvm_reserve_luns either returns 0 or -EBUSY and it is unlikely
> that return value would change and even if it does this can be
> updated.

If you propagate the result of nvm_reserve_luns(), the casual reader
will immediately understand that any possible faulty result is
returned. returning -EBUSY here might suggest you're overriding
whatever this function returns.

Re: [PATCH V2] lightnvm: if LUNs are already allocated fix return

[Frans Klaver]

On Tue, Jun 27, 2017 at 1:23 PM, Rakesh Pandit  wrote:
> On Tue, Jun 27, 2017 at 01:01:22PM +0200, Frans Klaver wrote:
>> On Tue, Jun 27, 2017 at 12:43 PM, Rakesh Pandit  wrote:
>> > While creating new device with NVM_DEV_CREATE if LUNs are already
>> > allocated ioctl would return -ENOMEM which is wrong.  This patch
>> > propagates -EBUSY from nvm_reserve_luns which is correct response.
>> >
>> > Fixes: ade69e243 ("lightnvm: merge gennvm with core")
>> > Signed-off-by: Rakesh Pandit 
>> > ---
>> >
>> > V2: return error code directly instead of using ret variable (Frans)
>> >
>> >  drivers/lightnvm/core.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
>> > index b8f82f5..c5d71c6 100644
>> > --- a/drivers/lightnvm/core.c
>> > +++ b/drivers/lightnvm/core.c
>> > @@ -253,7 +253,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct 
>> > nvm_ioctl_create *create)
>> > mutex_unlock(>mlock);
>> >
>> > if (nvm_reserve_luns(dev, s->lun_begin, s->lun_end))
>> > -   return -ENOMEM;
>> > +   return -EBUSY;
>>
>> Why aren't you propagating ret in this version?
>
> Well nvm_reserve_luns either returns 0 or -EBUSY and it is unlikely
> that return value would change and even if it does this can be
> updated.

If you propagate the result of nvm_reserve_luns(), the casual reader
will immediately understand that any possible faulty result is
returned. returning -EBUSY here might suggest you're overriding
whatever this function returns.

Re: [PATCH V2] lightnvm: if LUNs are already allocated fix return

[Frans Klaver]

On Tue, Jun 27, 2017 at 12:43 PM, Rakesh Pandit  wrote:
> While creating new device with NVM_DEV_CREATE if LUNs are already
> allocated ioctl would return -ENOMEM which is wrong.  This patch
> propagates -EBUSY from nvm_reserve_luns which is correct response.
>
> Fixes: ade69e243 ("lightnvm: merge gennvm with core")
> Signed-off-by: Rakesh Pandit 
> ---
>
> V2: return error code directly instead of using ret variable (Frans)
>
>  drivers/lightnvm/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index b8f82f5..c5d71c6 100644
> --- a/drivers/lightnvm/core.c
> +++ b/drivers/lightnvm/core.c
> @@ -253,7 +253,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct 
> nvm_ioctl_create *create)
> mutex_unlock(>mlock);
>
> if (nvm_reserve_luns(dev, s->lun_begin, s->lun_end))
> -   return -ENOMEM;
> +   return -EBUSY;

Why aren't you propagating ret in this version?

Re: [PATCH V2] lightnvm: if LUNs are already allocated fix return

[Frans Klaver]

On Tue, Jun 27, 2017 at 12:43 PM, Rakesh Pandit  wrote:
> While creating new device with NVM_DEV_CREATE if LUNs are already
> allocated ioctl would return -ENOMEM which is wrong.  This patch
> propagates -EBUSY from nvm_reserve_luns which is correct response.
>
> Fixes: ade69e243 ("lightnvm: merge gennvm with core")
> Signed-off-by: Rakesh Pandit 
> ---
>
> V2: return error code directly instead of using ret variable (Frans)
>
>  drivers/lightnvm/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index b8f82f5..c5d71c6 100644
> --- a/drivers/lightnvm/core.c
> +++ b/drivers/lightnvm/core.c
> @@ -253,7 +253,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct 
> nvm_ioctl_create *create)
> mutex_unlock(>mlock);
>
> if (nvm_reserve_luns(dev, s->lun_begin, s->lun_end))
> -   return -ENOMEM;
> +   return -EBUSY;

Why aren't you propagating ret in this version?

Re: [PATCH 4/4] Staging: rtl8712 : osdep_intf.h: fix macro coding style issue

[Frans Klaver]

On Tue, Jun 27, 2017 at 9:48 AM, Jaya Durga  wrote:
> Fix checkpatch.pl warning of the form "CHECK" Macro argument 'x'
> may be better as '(x)' to avoid precedence issues.
>
> Signed-off-by: Jaya Durga 
> ---
>  drivers/staging/rtl8712/osdep_intf.h | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8712/osdep_intf.h 
> b/drivers/staging/rtl8712/osdep_intf.h
> index 1985423..dac6aed 100644
> --- a/drivers/staging/rtl8712/osdep_intf.h
> +++ b/drivers/staging/rtl8712/osdep_intf.h
> @@ -29,7 +29,10 @@
>  #include "osdep_service.h"
>  #include "drv_types.h"
>
> -#define RND4(x)x) >> 2) + x) & 3) == 0) ?  0 : 1)) << 2)
> +static inline unsigned int RND4(unsigned int x)
> +{
> +   return (((x >> 2) + (((x & 3) == 0) ?  0 : 1)) << 2);
> +}

Looks like the checkpatch warning has already been addressed, or
checkpatch throws a false positive there.

I like this inline function better than the macro, since

RND(get_some_value());

may return funky results.

Frans

Re: [PATCH 4/4] Staging: rtl8712 : osdep_intf.h: fix macro coding style issue

[Frans Klaver]

On Tue, Jun 27, 2017 at 9:48 AM, Jaya Durga  wrote:
> Fix checkpatch.pl warning of the form "CHECK" Macro argument 'x'
> may be better as '(x)' to avoid precedence issues.
>
> Signed-off-by: Jaya Durga 
> ---
>  drivers/staging/rtl8712/osdep_intf.h | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8712/osdep_intf.h 
> b/drivers/staging/rtl8712/osdep_intf.h
> index 1985423..dac6aed 100644
> --- a/drivers/staging/rtl8712/osdep_intf.h
> +++ b/drivers/staging/rtl8712/osdep_intf.h
> @@ -29,7 +29,10 @@
>  #include "osdep_service.h"
>  #include "drv_types.h"
>
> -#define RND4(x)x) >> 2) + x) & 3) == 0) ?  0 : 1)) << 2)
> +static inline unsigned int RND4(unsigned int x)
> +{
> +   return (((x >> 2) + (((x & 3) == 0) ?  0 : 1)) << 2);
> +}

Looks like the checkpatch warning has already been addressed, or
checkpatch throws a false positive there.

I like this inline function better than the macro, since

RND(get_some_value());

may return funky results.

Frans

Re: [PATCH] ligtnvm: if LUNs are already allocated fix return

[Frans Klaver]

On Tue, Jun 27, 2017 at 10:39 AM, Matias Bjørling  wrote:
> From: Rakesh Pandit 
>
> While creating new device with NVM_DEV_CREATE if LUNs are already
> allocated ioctl would return -ENOMEM which is wrong.  This patch
> propagates -EBUSY from nvm_reserve_luns which is correct response.
>
> Fixes: ade69e243 ("lightnvm: merge gennvm with core")
> Signed-off-by: Rakesh Pandit 
> Signed-off-by: Matias Bjørling 
> ---
>  drivers/lightnvm/core.c | 11 ++-
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index b8f82f5..9ff348f 100644
> --- a/drivers/lightnvm/core.c
> +++ b/drivers/lightnvm/core.c
> @@ -235,7 +235,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct 
> nvm_ioctl_create *create)
> struct nvm_target *t;
> struct nvm_tgt_dev *tgt_dev;
> void *targetdata;
> -   int ret;
> +   int ret = 0;

Is there any way that you can reach a 'return ret' without having ret
set by some other assignment?


> tt = nvm_find_target_type(create->tgttype, 1);
> if (!tt) {
> @@ -252,8 +252,9 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct 
> nvm_ioctl_create *create)
> }
> mutex_unlock(>mlock);
>
> -   if (nvm_reserve_luns(dev, s->lun_begin, s->lun_end))
> -   return -ENOMEM;
> +   ret = nvm_reserve_luns(dev, s->lun_begin, s->lun_end);
> +   if (ret)
> +   goto err;

Why don't you return err straight away here?


> t = kmalloc(sizeof(struct nvm_target), GFP_KERNEL);
> if (!t) {
> @@ -314,8 +315,8 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct 
> nvm_ioctl_create *create)
> mutex_lock(>mlock);
> list_add_tail(>list, >targets);
> mutex_unlock(>mlock);
> -
> -   return 0;
> +err:
> +   return ret;

This should not be necessary. In any case, the de-init order should
always be the reverse of the init order, so we don't end up confused.

Frans

Re: [PATCH] ligtnvm: if LUNs are already allocated fix return

[Frans Klaver]

On Tue, Jun 27, 2017 at 10:39 AM, Matias Bjørling  wrote:
> From: Rakesh Pandit 
>
> While creating new device with NVM_DEV_CREATE if LUNs are already
> allocated ioctl would return -ENOMEM which is wrong.  This patch
> propagates -EBUSY from nvm_reserve_luns which is correct response.
>
> Fixes: ade69e243 ("lightnvm: merge gennvm with core")
> Signed-off-by: Rakesh Pandit 
> Signed-off-by: Matias Bjørling 
> ---
>  drivers/lightnvm/core.c | 11 ++-
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index b8f82f5..9ff348f 100644
> --- a/drivers/lightnvm/core.c
> +++ b/drivers/lightnvm/core.c
> @@ -235,7 +235,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct 
> nvm_ioctl_create *create)
> struct nvm_target *t;
> struct nvm_tgt_dev *tgt_dev;
> void *targetdata;
> -   int ret;
> +   int ret = 0;

Is there any way that you can reach a 'return ret' without having ret
set by some other assignment?


> tt = nvm_find_target_type(create->tgttype, 1);
> if (!tt) {
> @@ -252,8 +252,9 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct 
> nvm_ioctl_create *create)
> }
> mutex_unlock(>mlock);
>
> -   if (nvm_reserve_luns(dev, s->lun_begin, s->lun_end))
> -   return -ENOMEM;
> +   ret = nvm_reserve_luns(dev, s->lun_begin, s->lun_end);
> +   if (ret)
> +   goto err;

Why don't you return err straight away here?


> t = kmalloc(sizeof(struct nvm_target), GFP_KERNEL);
> if (!t) {
> @@ -314,8 +315,8 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct 
> nvm_ioctl_create *create)
> mutex_lock(>mlock);
> list_add_tail(>list, >targets);
> mutex_unlock(>mlock);
> -
> -   return 0;
> +err:
> +   return ret;

This should not be necessary. In any case, the de-init order should
always be the reverse of the init order, so we don't end up confused.

Frans

Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]

[Frans Klaver]

On Fri, Jun 23, 2017 at 07:37:28PM -0400, Julia Lawall wrote:
> 
> 
> On Sat, 24 Jun 2017, Frans Klaver wrote:
> 
> > Hm. For some reason the great mail filtering scheme decided to push
> > this past my inbox :-/
> >
> > On Sat, Jun 17, 2017 at 12:44 AM, Joe Perches <j...@perches.com> wrote:
> > > On Fri, 2017-06-16 at 19:45 +0200, Frans Klaver wrote:
> > >> The header field in struct pd_message is declared as an __le16 type. The
> > >> data in the message is supposed to be little endian. This means we don't
> > >> have to go and shift the individual bytes into position when we're
> > >> filling the buffer, we can just copy the contents right away. As an
> > >> added benefit we don't get fishy results on big endian systems anymore.
> > >
> > > Thanks for pointing this out.
> > >
> > > There are several instances of this class of error.
> >
> > There are other smells around __(le|be) types that show up in staging
> > that might be worth checking in the rest of the kernel as well. e.g.
> > converting to cpu and storing it back into itself (possibly with its
> > bytes reversed), direct assignments without conversion and what else
> > you might have. sparse obviously already flags anything fishy going on
> > with these types, but cannot distinguish between the classes of
> > errors. I'll need to acquaint myself with spatch a bit more to be able
> > to track that down.
> 
> If you have concrete code examples, even fake ones, illustrating a class
> of problem, then that would be great.

Alright, I'll describe two fairly simple cases for starters.

One class of issue that I have on top of mind is simply

__le16 val;

val = le16_to_cpu(val);

The problem there obviously being that val is supposed to be guaranteed
little endian. Sparse will throw a warning at this. It may also appear
as (or be 'fixed' as)

__le16 val;

le16_to_cpus(val);

Sparse doesn't flag this second version as an issue, while it causes the
same problem. It is especially a potential problem when the value is
stored in driver data.

Another smell that is prevalent, at least in staging, is

u16 in;
u16 out;

out = cpu_to_le16(in);

or in one instance (drivers/staging/fbtft/fbtft-io.c) I saw

u64 tmp;

*(u64*)dst = cpu_to_be64(tmp);

Now these aren't necessarily problematic. Usually this typo of code is
preparing the data to be sent out in a specific byte ordering, but again
issues may arise if this specifically ordered data is stored somewhere.

I'll leave it at that for now. 

Frans

Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]

[Frans Klaver]

On Fri, Jun 23, 2017 at 07:37:28PM -0400, Julia Lawall wrote:
> 
> 
> On Sat, 24 Jun 2017, Frans Klaver wrote:
> 
> > Hm. For some reason the great mail filtering scheme decided to push
> > this past my inbox :-/
> >
> > On Sat, Jun 17, 2017 at 12:44 AM, Joe Perches  wrote:
> > > On Fri, 2017-06-16 at 19:45 +0200, Frans Klaver wrote:
> > >> The header field in struct pd_message is declared as an __le16 type. The
> > >> data in the message is supposed to be little endian. This means we don't
> > >> have to go and shift the individual bytes into position when we're
> > >> filling the buffer, we can just copy the contents right away. As an
> > >> added benefit we don't get fishy results on big endian systems anymore.
> > >
> > > Thanks for pointing this out.
> > >
> > > There are several instances of this class of error.
> >
> > There are other smells around __(le|be) types that show up in staging
> > that might be worth checking in the rest of the kernel as well. e.g.
> > converting to cpu and storing it back into itself (possibly with its
> > bytes reversed), direct assignments without conversion and what else
> > you might have. sparse obviously already flags anything fishy going on
> > with these types, but cannot distinguish between the classes of
> > errors. I'll need to acquaint myself with spatch a bit more to be able
> > to track that down.
> 
> If you have concrete code examples, even fake ones, illustrating a class
> of problem, then that would be great.

Alright, I'll describe two fairly simple cases for starters.

One class of issue that I have on top of mind is simply

__le16 val;

val = le16_to_cpu(val);

The problem there obviously being that val is supposed to be guaranteed
little endian. Sparse will throw a warning at this. It may also appear
as (or be 'fixed' as)

__le16 val;

le16_to_cpus(val);

Sparse doesn't flag this second version as an issue, while it causes the
same problem. It is especially a potential problem when the value is
stored in driver data.

Another smell that is prevalent, at least in staging, is

u16 in;
u16 out;

out = cpu_to_le16(in);

or in one instance (drivers/staging/fbtft/fbtft-io.c) I saw

u64 tmp;

*(u64*)dst = cpu_to_be64(tmp);

Now these aren't necessarily problematic. Usually this typo of code is
preparing the data to be sent out in a specific byte ordering, but again
issues may arise if this specifically ordered data is stored somewhere.

I'll leave it at that for now. 

Frans

Re: [PATCH v4] crypto: sun4i-ss: support the Security System PRNG

[Frans Klaver]

Hi,

On Mon, Jun 26, 2017 at 2:20 PM, Corentin Labbe
 wrote:
> The Security System have a PRNG, this patch add support for it via
> crypto_rng.

s,have,has,
s,add,adds,

>
> Signed-off-by: Corentin Labbe 
> ---
>
> Changes since v3 (note: the v3 miss changes and version tag sorry)
> - Replaced all len values with bits / BITS_PER_LONG or BITS_PER_BYTE
>
> Changes since v2
>  - converted to crypto_rng
>
> Changes since v1:
>  - Replaced all spin_lock_bh by simple spin_lock
>  - Removed handling of size not modulo 4 which will never happen
>  - Added add_random_ready_callback()
>
>  drivers/crypto/Kconfig  |  8 +
>  drivers/crypto/sunxi-ss/Makefile|  1 +
>  drivers/crypto/sunxi-ss/sun4i-ss-core.c | 30 ++
>  drivers/crypto/sunxi-ss/sun4i-ss-prng.c | 56 
> +
>  drivers/crypto/sunxi-ss/sun4i-ss.h  | 11 +++
>  5 files changed, 106 insertions(+)
>  create mode 100644 drivers/crypto/sunxi-ss/sun4i-ss-prng.c
>
> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index ab82536d64e2..bde0b102eb70 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -618,6 +618,14 @@ config CRYPTO_DEV_SUN4I_SS
>   To compile this driver as a module, choose M here: the module
>   will be called sun4i-ss.
>
> +config CRYPTO_DEV_SUN4I_SS_PRNG
> +   bool "Support for Allwinner Security System PRNG"
> +   depends on CRYPTO_DEV_SUN4I_SS
> +   select CRYPTO_RNG
> +   help
> + Select this option if you to provides kernel-side support for
> + the Pseudo-Random Number Generator found in the Security System.

This sentence does not parse. "Select this option if you want to
provide kernel-side for ...". Alternatively: "This option enables
kernel-side support for ..."

Frans

Re: [PATCH v4] crypto: sun4i-ss: support the Security System PRNG

[Frans Klaver]

Hi,

On Mon, Jun 26, 2017 at 2:20 PM, Corentin Labbe
 wrote:
> The Security System have a PRNG, this patch add support for it via
> crypto_rng.

s,have,has,
s,add,adds,

>
> Signed-off-by: Corentin Labbe 
> ---
>
> Changes since v3 (note: the v3 miss changes and version tag sorry)
> - Replaced all len values with bits / BITS_PER_LONG or BITS_PER_BYTE
>
> Changes since v2
>  - converted to crypto_rng
>
> Changes since v1:
>  - Replaced all spin_lock_bh by simple spin_lock
>  - Removed handling of size not modulo 4 which will never happen
>  - Added add_random_ready_callback()
>
>  drivers/crypto/Kconfig  |  8 +
>  drivers/crypto/sunxi-ss/Makefile|  1 +
>  drivers/crypto/sunxi-ss/sun4i-ss-core.c | 30 ++
>  drivers/crypto/sunxi-ss/sun4i-ss-prng.c | 56 
> +
>  drivers/crypto/sunxi-ss/sun4i-ss.h  | 11 +++
>  5 files changed, 106 insertions(+)
>  create mode 100644 drivers/crypto/sunxi-ss/sun4i-ss-prng.c
>
> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index ab82536d64e2..bde0b102eb70 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -618,6 +618,14 @@ config CRYPTO_DEV_SUN4I_SS
>   To compile this driver as a module, choose M here: the module
>   will be called sun4i-ss.
>
> +config CRYPTO_DEV_SUN4I_SS_PRNG
> +   bool "Support for Allwinner Security System PRNG"
> +   depends on CRYPTO_DEV_SUN4I_SS
> +   select CRYPTO_RNG
> +   help
> + Select this option if you to provides kernel-side support for
> + the Pseudo-Random Number Generator found in the Security System.

This sentence does not parse. "Select this option if you want to
provide kernel-side for ...". Alternatively: "This option enables
kernel-side support for ..."

Frans

Re: [PATCH] Staging : rts5208 : checkpatch.pl fixes

[Frans Klaver]

On Mon, Jun 26, 2017 at 11:12 AM, Simo Koskinen  wrote:
> On Fri, Jun 23, 2017 at 5:59 PM, Joe Perches  wrote:
>> On Fri, 2017-06-23 at 15:55 +0200, Simo Koskinen wrote:
>>> Fixed some issues reported by checkpatch.pl script.
>> []
>>> diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
>>> index b8177f5..ceef5fc 100644
>>> --- a/drivers/staging/rts5208/rtsx.c
>>> +++ b/drivers/staging/rts5208/rtsx.c
>>> @@ -1009,7 +1009,7 @@ static void rtsx_remove(struct pci_dev *pci)
>>>  {
>>>   struct rtsx_dev *dev = pci_get_drvdata(pci);
>>>
>>> - dev_info(>dev, "rtsx_remove() called\n");
>>> + dev_info(>dev, "%s called\n", "rtsx_remove()");
>>
>> This would be better as dev_dbg
> True, I can change that...
>
>
>>>
>>>   quiesce_and_remove_host(dev);
>>>   release_everything(dev);
>>> diff --git a/drivers/staging/rts5208/rtsx_chip.c 
>>> b/drivers/staging/rts5208/rtsx_chip.c
>>> index 7f4107b..892b97a 100644
>>> --- a/drivers/staging/rts5208/rtsx_chip.c
>>> +++ b/drivers/staging/rts5208/rtsx_chip.c
>>> @@ -616,8 +616,10 @@ int rtsx_reset_chip(struct rtsx_chip *chip)
>>>   else
>>>   retval = rtsx_pre_handle_sdio_new(chip);
>>>
>>> - dev_dbg(rtsx_dev(chip), "chip->need_reset = 0x%x 
>>> (rtsx_reset_chip)\n",
>>> - (unsigned int)(chip->need_reset));
>>> + dev_dbg(rtsx_dev(chip), "%s = 0x%x (%s)\n",
>>> + "chip->need_reset",
>>> + (unsigned int)(chip->need_reset),
>>> + "rtsx_reset_chip");
>>
>> This and other changes that take part of the format
>> and convert them to '"%s", substrings' are not good.
>> checkpatch doesn't emit a warning for long formats.
> The reason for changes were the following warning when run the
> checkpatch.pl script:
>
> WARNING: Prefer using '"%s...", __func__' to using 'rtsx_reset_chip',
> this function's name, in a string
> #619: FILE: rtsx_chip.c:619:
> +dev_dbg(rtsx_dev(chip), "chip->need_reset = 0x%x 
> (rtsx_reset_chip)\n",
>
> So it's not a good idea to fix these warnings?

The warning suggests you change this to

dev_dbg(rtsx_dev(chip), "chip->need_reset = 0x%x (%s)\n", (unsigned
int)(chip->need_reset), __func__);

It doesn't mention the chip->need_reset part. The reason is simply
that when the function name changes, this doesn't have to be updated.
That same reasoning doesn't hold up for when something changes in
"chip->need_reset".

Frans

Re: [PATCH] Staging : rts5208 : checkpatch.pl fixes

[Frans Klaver]

On Mon, Jun 26, 2017 at 11:12 AM, Simo Koskinen  wrote:
> On Fri, Jun 23, 2017 at 5:59 PM, Joe Perches  wrote:
>> On Fri, 2017-06-23 at 15:55 +0200, Simo Koskinen wrote:
>>> Fixed some issues reported by checkpatch.pl script.
>> []
>>> diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
>>> index b8177f5..ceef5fc 100644
>>> --- a/drivers/staging/rts5208/rtsx.c
>>> +++ b/drivers/staging/rts5208/rtsx.c
>>> @@ -1009,7 +1009,7 @@ static void rtsx_remove(struct pci_dev *pci)
>>>  {
>>>   struct rtsx_dev *dev = pci_get_drvdata(pci);
>>>
>>> - dev_info(>dev, "rtsx_remove() called\n");
>>> + dev_info(>dev, "%s called\n", "rtsx_remove()");
>>
>> This would be better as dev_dbg
> True, I can change that...
>
>
>>>
>>>   quiesce_and_remove_host(dev);
>>>   release_everything(dev);
>>> diff --git a/drivers/staging/rts5208/rtsx_chip.c 
>>> b/drivers/staging/rts5208/rtsx_chip.c
>>> index 7f4107b..892b97a 100644
>>> --- a/drivers/staging/rts5208/rtsx_chip.c
>>> +++ b/drivers/staging/rts5208/rtsx_chip.c
>>> @@ -616,8 +616,10 @@ int rtsx_reset_chip(struct rtsx_chip *chip)
>>>   else
>>>   retval = rtsx_pre_handle_sdio_new(chip);
>>>
>>> - dev_dbg(rtsx_dev(chip), "chip->need_reset = 0x%x 
>>> (rtsx_reset_chip)\n",
>>> - (unsigned int)(chip->need_reset));
>>> + dev_dbg(rtsx_dev(chip), "%s = 0x%x (%s)\n",
>>> + "chip->need_reset",
>>> + (unsigned int)(chip->need_reset),
>>> + "rtsx_reset_chip");
>>
>> This and other changes that take part of the format
>> and convert them to '"%s", substrings' are not good.
>> checkpatch doesn't emit a warning for long formats.
> The reason for changes were the following warning when run the
> checkpatch.pl script:
>
> WARNING: Prefer using '"%s...", __func__' to using 'rtsx_reset_chip',
> this function's name, in a string
> #619: FILE: rtsx_chip.c:619:
> +dev_dbg(rtsx_dev(chip), "chip->need_reset = 0x%x 
> (rtsx_reset_chip)\n",
>
> So it's not a good idea to fix these warnings?

The warning suggests you change this to

dev_dbg(rtsx_dev(chip), "chip->need_reset = 0x%x (%s)\n", (unsigned
int)(chip->need_reset), __func__);

It doesn't mention the chip->need_reset part. The reason is simply
that when the function name changes, this doesn't have to be updated.
That same reasoning doesn't hold up for when something changes in
"chip->need_reset".

Frans

Re: [PATCH] staging: sm750fb: always take the lock

[Frans Klaver]

On Mon, Jun 26, 2017 at 11:11 AM, Geert Uytterhoeven
 wrote:
> On Mon, Jun 26, 2017 at 7:45 AM, AbdAllah-MEZITI
>  wrote:
>> This patch
>> - will always take the lock
>
> Why?
>
> "The current code only takes the lock if multiple instances are in use.
>  This is error-prone, and confuses static analyzers.
>  As taking the lock in case of a single instance is harmful and cheap,
>  change the code to always take the lock."

I would argue that it's not harmful, lest people get confused about
it. And I agree that this explanation is much more useful than just
mentioning the warnings that you saw.

Thanks,
Frans

Re: [PATCH] staging: sm750fb: always take the lock

[Frans Klaver]

On Mon, Jun 26, 2017 at 11:11 AM, Geert Uytterhoeven
 wrote:
> On Mon, Jun 26, 2017 at 7:45 AM, AbdAllah-MEZITI
>  wrote:
>> This patch
>> - will always take the lock
>
> Why?
>
> "The current code only takes the lock if multiple instances are in use.
>  This is error-prone, and confuses static analyzers.
>  As taking the lock in case of a single instance is harmful and cheap,
>  change the code to always take the lock."

I would argue that it's not harmful, lest people get confused about
it. And I agree that this explanation is much more useful than just
mentioning the warnings that you saw.

Thanks,
Frans

Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]

[Frans Klaver]

On Sat, Jun 24, 2017 at 1:37 AM, Julia Lawall <julia.law...@lip6.fr> wrote:
>
>
> On Sat, 24 Jun 2017, Frans Klaver wrote:
>
>> Hm. For some reason the great mail filtering scheme decided to push
>> this past my inbox :-/
>>
>> On Sat, Jun 17, 2017 at 12:44 AM, Joe Perches <j...@perches.com> wrote:
>> > On Fri, 2017-06-16 at 19:45 +0200, Frans Klaver wrote:
>> >> The header field in struct pd_message is declared as an __le16 type. The
>> >> data in the message is supposed to be little endian. This means we don't
>> >> have to go and shift the individual bytes into position when we're
>> >> filling the buffer, we can just copy the contents right away. As an
>> >> added benefit we don't get fishy results on big endian systems anymore.
>> >
>> > Thanks for pointing this out.
>> >
>> > There are several instances of this class of error.
>>
>> There are other smells around __(le|be) types that show up in staging
>> that might be worth checking in the rest of the kernel as well. e.g.
>> converting to cpu and storing it back into itself (possibly with its
>> bytes reversed), direct assignments without conversion and what else
>> you might have. sparse obviously already flags anything fishy going on
>> with these types, but cannot distinguish between the classes of
>> errors. I'll need to acquaint myself with spatch a bit more to be able
>> to track that down.
>
> If you have concrete code examples, even fake ones, illustrating a class
> of problem, then that would be great.

I'll see if I can produce some somewhere this week.

Thanks,
Frans

Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]

[Frans Klaver]

On Sat, Jun 24, 2017 at 1:37 AM, Julia Lawall  wrote:
>
>
> On Sat, 24 Jun 2017, Frans Klaver wrote:
>
>> Hm. For some reason the great mail filtering scheme decided to push
>> this past my inbox :-/
>>
>> On Sat, Jun 17, 2017 at 12:44 AM, Joe Perches  wrote:
>> > On Fri, 2017-06-16 at 19:45 +0200, Frans Klaver wrote:
>> >> The header field in struct pd_message is declared as an __le16 type. The
>> >> data in the message is supposed to be little endian. This means we don't
>> >> have to go and shift the individual bytes into position when we're
>> >> filling the buffer, we can just copy the contents right away. As an
>> >> added benefit we don't get fishy results on big endian systems anymore.
>> >
>> > Thanks for pointing this out.
>> >
>> > There are several instances of this class of error.
>>
>> There are other smells around __(le|be) types that show up in staging
>> that might be worth checking in the rest of the kernel as well. e.g.
>> converting to cpu and storing it back into itself (possibly with its
>> bytes reversed), direct assignments without conversion and what else
>> you might have. sparse obviously already flags anything fishy going on
>> with these types, but cannot distinguish between the classes of
>> errors. I'll need to acquaint myself with spatch a bit more to be able
>> to track that down.
>
> If you have concrete code examples, even fake ones, illustrating a class
> of problem, then that would be great.

I'll see if I can produce some somewhere this week.

Thanks,
Frans

Re: [PATCH] staging: sm750fb: always take the lock

[Frans Klaver]

There's no version number. Which one is the correct one?

On Mon, Jun 26, 2017 at 7:45 AM, AbdAllah-MEZITI
 wrote:
> This patch
> - will always take the lock
> - fix the sparse warning:
> drivers/staging/sm750fb/sm750.c:159:13: warning: context imbalance in 
> 'lynxfb_ops_fillrect' - different lock contexts for basic block
> drivers/staging/sm750fb/sm750.c:231:9: warning: context imbalance in 
> 'lynxfb_ops_copyarea' - different lock contexts for basic block
> drivers/staging/sm750fb/sm750.c:235:13: warning: context imbalance in 
> 'lynxfb_ops_imageblit' - different lock contexts for basic block
>
> Signed-off-by: AbdAllah MEZITI 
> ---
>  drivers/staging/sm750fb/sm750.c | 18 ++
>  1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
> index 386d4ad..4a22190 100644
> --- a/drivers/staging/sm750fb/sm750.c
> +++ b/drivers/staging/sm750fb/sm750.c
> @@ -186,16 +186,14 @@ static void lynxfb_ops_fillrect(struct fb_info *info,
>  * If not use spin_lock,system will die if user load driver
>  * and immediately unload driver frequently (dual)
>  */
> -   if (sm750_dev->fb_count > 1)
> -   spin_lock(_dev->slock);
> +   spin_lock(_dev->slock);
>
> sm750_dev->accel.de_fillrect(_dev->accel,
>  base, pitch, Bpp,
>  region->dx, region->dy,
>  region->width, region->height,
>  color, rop);
> -   if (sm750_dev->fb_count > 1)
> -   spin_unlock(_dev->slock);
> +   spin_unlock(_dev->slock);
>  }
>
>  static void lynxfb_ops_copyarea(struct fb_info *info,
> @@ -220,16 +218,14 @@ static void lynxfb_ops_copyarea(struct fb_info *info,
>  * If not use spin_lock, system will die if user load driver
>  * and immediately unload driver frequently (dual)
>  */
> -   if (sm750_dev->fb_count > 1)
> -   spin_lock(_dev->slock);
> +   spin_lock(_dev->slock);
>
> sm750_dev->accel.de_copyarea(_dev->accel,
>  base, pitch, region->sx, region->sy,
>  base, pitch, Bpp, region->dx, region->dy,
>  region->width, region->height,
>  HW_ROP2_COPY);
> -   if (sm750_dev->fb_count > 1)
> -   spin_unlock(_dev->slock);
> +   spin_unlock(_dev->slock);
>  }
>
>  static void lynxfb_ops_imageblit(struct fb_info *info,
> @@ -269,8 +265,7 @@ static void lynxfb_ops_imageblit(struct fb_info *info,
>  * If not use spin_lock, system will die if user load driver
>  * and immediately unload driver frequently (dual)
>  */
> -   if (sm750_dev->fb_count > 1)
> -   spin_lock(_dev->slock);
> +   spin_lock(_dev->slock);
>
> sm750_dev->accel.de_imageblit(_dev->accel,
>   image->data, image->width >> 3, 0,
> @@ -278,8 +273,7 @@ static void lynxfb_ops_imageblit(struct fb_info *info,
>   image->dx, image->dy,
>   image->width, image->height,
>   fgcol, bgcol, HW_ROP2_COPY);
> -   if (sm750_dev->fb_count > 1)
> -   spin_unlock(_dev->slock);
> +   spin_unlock(_dev->slock);
>  }
>
>  static int lynxfb_ops_pan_display(struct fb_var_screeninfo *var,
> --
> 2.7.4
>

Re: [PATCH] staging: sm750fb: always take the lock

[Frans Klaver]

There's no version number. Which one is the correct one?

On Mon, Jun 26, 2017 at 7:45 AM, AbdAllah-MEZITI
 wrote:
> This patch
> - will always take the lock
> - fix the sparse warning:
> drivers/staging/sm750fb/sm750.c:159:13: warning: context imbalance in 
> 'lynxfb_ops_fillrect' - different lock contexts for basic block
> drivers/staging/sm750fb/sm750.c:231:9: warning: context imbalance in 
> 'lynxfb_ops_copyarea' - different lock contexts for basic block
> drivers/staging/sm750fb/sm750.c:235:13: warning: context imbalance in 
> 'lynxfb_ops_imageblit' - different lock contexts for basic block
>
> Signed-off-by: AbdAllah MEZITI 
> ---
>  drivers/staging/sm750fb/sm750.c | 18 ++
>  1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
> index 386d4ad..4a22190 100644
> --- a/drivers/staging/sm750fb/sm750.c
> +++ b/drivers/staging/sm750fb/sm750.c
> @@ -186,16 +186,14 @@ static void lynxfb_ops_fillrect(struct fb_info *info,
>  * If not use spin_lock,system will die if user load driver
>  * and immediately unload driver frequently (dual)
>  */
> -   if (sm750_dev->fb_count > 1)
> -   spin_lock(_dev->slock);
> +   spin_lock(_dev->slock);
>
> sm750_dev->accel.de_fillrect(_dev->accel,
>  base, pitch, Bpp,
>  region->dx, region->dy,
>  region->width, region->height,
>  color, rop);
> -   if (sm750_dev->fb_count > 1)
> -   spin_unlock(_dev->slock);
> +   spin_unlock(_dev->slock);
>  }
>
>  static void lynxfb_ops_copyarea(struct fb_info *info,
> @@ -220,16 +218,14 @@ static void lynxfb_ops_copyarea(struct fb_info *info,
>  * If not use spin_lock, system will die if user load driver
>  * and immediately unload driver frequently (dual)
>  */
> -   if (sm750_dev->fb_count > 1)
> -   spin_lock(_dev->slock);
> +   spin_lock(_dev->slock);
>
> sm750_dev->accel.de_copyarea(_dev->accel,
>  base, pitch, region->sx, region->sy,
>  base, pitch, Bpp, region->dx, region->dy,
>  region->width, region->height,
>  HW_ROP2_COPY);
> -   if (sm750_dev->fb_count > 1)
> -   spin_unlock(_dev->slock);
> +   spin_unlock(_dev->slock);
>  }
>
>  static void lynxfb_ops_imageblit(struct fb_info *info,
> @@ -269,8 +265,7 @@ static void lynxfb_ops_imageblit(struct fb_info *info,
>  * If not use spin_lock, system will die if user load driver
>  * and immediately unload driver frequently (dual)
>  */
> -   if (sm750_dev->fb_count > 1)
> -   spin_lock(_dev->slock);
> +   spin_lock(_dev->slock);
>
> sm750_dev->accel.de_imageblit(_dev->accel,
>   image->data, image->width >> 3, 0,
> @@ -278,8 +273,7 @@ static void lynxfb_ops_imageblit(struct fb_info *info,
>   image->dx, image->dy,
>   image->width, image->height,
>   fgcol, bgcol, HW_ROP2_COPY);
> -   if (sm750_dev->fb_count > 1)
> -   spin_unlock(_dev->slock);
> +   spin_unlock(_dev->slock);
>  }
>
>  static int lynxfb_ops_pan_display(struct fb_var_screeninfo *var,
> --
> 2.7.4
>

Re: [PATCH] staging: sm750fb: always take the lock

[Frans Klaver]

On Sun, Jun 25, 2017 at 11:39 PM, AbdAllah-MEZITI
 wrote:
> Subject: [PATCH] staging: sm750fb: always take the lock

When sending a new version of your patch, include a version number:

Subject: [PATCH V2] staging: ...

Frans

Re: [PATCH] staging: sm750fb: always take the lock

[Frans Klaver]

On Sun, Jun 25, 2017 at 11:39 PM, AbdAllah-MEZITI
 wrote:
> Subject: [PATCH] staging: sm750fb: always take the lock

When sending a new version of your patch, include a version number:

Subject: [PATCH V2] staging: ...

Frans

Re: [PATCH] staging: sm750fb: fix sparce warning.

[Frans Klaver]

On 25 June 2017 21:10:36 CEST, AbdAllah-MEZITI  
wrote:
>This patch fixes the following sparce warnings: different lock contexts
>for basic block.
>
>drivers/staging/sm750fb//sm750.c:159:13: warning: context imbalance in
>'lynxfb_ops_fillrect' - different lock contexts for basic block
>drivers/staging/sm750fb//sm750.c:231:9: warning: context imbalance in
>'lynxfb_ops_copyarea' - different lock contexts for basic block
>drivers/staging/sm750fb//sm750.c:235:13: warning: context imbalance in
>'lynxfb_ops_imageblit' - different lock contexts for basic block
>
>Signed-off-by: AbdAllah-MEZITI 
>---
>drivers/staging/sm750fb/sm750.c | 69
>-
> 1 file changed, 47 insertions(+), 22 deletions(-)
>
>diff --git a/drivers/staging/sm750fb/sm750.c
>b/drivers/staging/sm750fb/sm750.c
>index 664c220..5494a29 100644
>--- a/drivers/staging/sm750fb/sm750.c
>+++ b/drivers/staging/sm750fb/sm750.c
>@@ -186,16 +186,24 @@ static void lynxfb_ops_fillrect(struct fb_info
>*info,
>* If not use spin_lock,system will die if user load driver
>* and immediately unload driver frequently (dual)
>*/
>-  if (sm750_dev->fb_count > 1)
>+  if (sm750_dev->fb_count > 1) {
>   spin_lock(_dev->slock);
> 
>-  sm750_dev->accel.de_fillrect(_dev->accel,
>-   base, pitch, Bpp,
>-   region->dx, region->dy,
>-   region->width, region->height,
>-   color, rop);
>-  if (sm750_dev->fb_count > 1)
>+  sm750_dev->accel.de_fillrect(_dev->accel,
>+   base, pitch, Bpp,
>+   region->dx, region->dy,
>+   region->width, region->height,
>+   color, rop);
>+
>   spin_unlock(_dev->slock);
>+  } else {
>+  sm750_dev->accel.de_fillrect(_dev->accel,
>+   base, pitch, Bpp,
>+   region->dx, region->dy,
>+   region->width, region->height,
>+   color, rop);
>+  }
>+
> }
> 
> static void lynxfb_ops_copyarea(struct fb_info *info,
>@@ -220,16 +228,24 @@ static void lynxfb_ops_copyarea(struct fb_info
>*info,
>* If not use spin_lock, system will die if user load driver
>* and immediately unload driver frequently (dual)
>*/
>-  if (sm750_dev->fb_count > 1)
>+  if (sm750_dev->fb_count > 1) {
>   spin_lock(_dev->slock);
> 
>-  sm750_dev->accel.de_copyarea(_dev->accel,
>-   base, pitch, region->sx, region->sy,
>-   base, pitch, Bpp, region->dx, region->dy,
>-   region->width, region->height,
>-   HW_ROP2_COPY);
>-  if (sm750_dev->fb_count > 1)
>+  sm750_dev->accel.de_copyarea(_dev->accel,
>+   base, pitch, region->sx, 
>region->sy,
>+   base, pitch, Bpp, region->dx, 
>region->dy,
>+   region->width, region->height,
>+   HW_ROP2_COPY);
>+
>   spin_unlock(_dev->slock);
>+  } else {
>+  sm750_dev->accel.de_copyarea(_dev->accel,
>+   base, pitch, region->sx, 
>region->sy,
>+   base, pitch, Bpp, region->dx, 
>region->dy,
>+   region->width, region->height,
>+   HW_ROP2_COPY);
>+  }
>+
> }
> 
> static void lynxfb_ops_imageblit(struct fb_info *info,
>@@ -269,17 +285,26 @@ static void lynxfb_ops_imageblit(struct fb_info
>*info,
>* If not use spin_lock, system will die if user load driver
>* and immediately unload driver frequently (dual)
>*/
>-  if (sm750_dev->fb_count > 1)
>+  if (sm750_dev->fb_count > 1) {
>   spin_lock(_dev->slock);
> 
>-  sm750_dev->accel.de_imageblit(_dev->accel,
>-image->data, image->width >> 3, 0,
>-base, pitch, Bpp,
>-image->dx, image->dy,
>-image->width, image->height,
>-fgcol, bgcol, HW_ROP2_COPY);
>-  if (sm750_dev->fb_count > 1)
>+  sm750_dev->accel.de_imageblit(_dev->accel,
>+image->data, image->width >> 3, 0,
>+base, pitch, Bpp,
>+  

Re: [PATCH] staging: sm750fb: fix sparce warning.

[Frans Klaver]

On 25 June 2017 21:10:36 CEST, AbdAllah-MEZITI  
wrote:
>This patch fixes the following sparce warnings: different lock contexts
>for basic block.
>
>drivers/staging/sm750fb//sm750.c:159:13: warning: context imbalance in
>'lynxfb_ops_fillrect' - different lock contexts for basic block
>drivers/staging/sm750fb//sm750.c:231:9: warning: context imbalance in
>'lynxfb_ops_copyarea' - different lock contexts for basic block
>drivers/staging/sm750fb//sm750.c:235:13: warning: context imbalance in
>'lynxfb_ops_imageblit' - different lock contexts for basic block
>
>Signed-off-by: AbdAllah-MEZITI 
>---
>drivers/staging/sm750fb/sm750.c | 69
>-
> 1 file changed, 47 insertions(+), 22 deletions(-)
>
>diff --git a/drivers/staging/sm750fb/sm750.c
>b/drivers/staging/sm750fb/sm750.c
>index 664c220..5494a29 100644
>--- a/drivers/staging/sm750fb/sm750.c
>+++ b/drivers/staging/sm750fb/sm750.c
>@@ -186,16 +186,24 @@ static void lynxfb_ops_fillrect(struct fb_info
>*info,
>* If not use spin_lock,system will die if user load driver
>* and immediately unload driver frequently (dual)
>*/
>-  if (sm750_dev->fb_count > 1)
>+  if (sm750_dev->fb_count > 1) {
>   spin_lock(_dev->slock);
> 
>-  sm750_dev->accel.de_fillrect(_dev->accel,
>-   base, pitch, Bpp,
>-   region->dx, region->dy,
>-   region->width, region->height,
>-   color, rop);
>-  if (sm750_dev->fb_count > 1)
>+  sm750_dev->accel.de_fillrect(_dev->accel,
>+   base, pitch, Bpp,
>+   region->dx, region->dy,
>+   region->width, region->height,
>+   color, rop);
>+
>   spin_unlock(_dev->slock);
>+  } else {
>+  sm750_dev->accel.de_fillrect(_dev->accel,
>+   base, pitch, Bpp,
>+   region->dx, region->dy,
>+   region->width, region->height,
>+   color, rop);
>+  }
>+
> }
> 
> static void lynxfb_ops_copyarea(struct fb_info *info,
>@@ -220,16 +228,24 @@ static void lynxfb_ops_copyarea(struct fb_info
>*info,
>* If not use spin_lock, system will die if user load driver
>* and immediately unload driver frequently (dual)
>*/
>-  if (sm750_dev->fb_count > 1)
>+  if (sm750_dev->fb_count > 1) {
>   spin_lock(_dev->slock);
> 
>-  sm750_dev->accel.de_copyarea(_dev->accel,
>-   base, pitch, region->sx, region->sy,
>-   base, pitch, Bpp, region->dx, region->dy,
>-   region->width, region->height,
>-   HW_ROP2_COPY);
>-  if (sm750_dev->fb_count > 1)
>+  sm750_dev->accel.de_copyarea(_dev->accel,
>+   base, pitch, region->sx, 
>region->sy,
>+   base, pitch, Bpp, region->dx, 
>region->dy,
>+   region->width, region->height,
>+   HW_ROP2_COPY);
>+
>   spin_unlock(_dev->slock);
>+  } else {
>+  sm750_dev->accel.de_copyarea(_dev->accel,
>+   base, pitch, region->sx, 
>region->sy,
>+   base, pitch, Bpp, region->dx, 
>region->dy,
>+   region->width, region->height,
>+   HW_ROP2_COPY);
>+  }
>+
> }
> 
> static void lynxfb_ops_imageblit(struct fb_info *info,
>@@ -269,17 +285,26 @@ static void lynxfb_ops_imageblit(struct fb_info
>*info,
>* If not use spin_lock, system will die if user load driver
>* and immediately unload driver frequently (dual)
>*/
>-  if (sm750_dev->fb_count > 1)
>+  if (sm750_dev->fb_count > 1) {
>   spin_lock(_dev->slock);
> 
>-  sm750_dev->accel.de_imageblit(_dev->accel,
>-image->data, image->width >> 3, 0,
>-base, pitch, Bpp,
>-image->dx, image->dy,
>-image->width, image->height,
>-fgcol, bgcol, HW_ROP2_COPY);
>-  if (sm750_dev->fb_count > 1)
>+  sm750_dev->accel.de_imageblit(_dev->accel,
>+image->data, image->width >> 3, 0,
>+base, pitch, Bpp,
>+image->dx, image->dy,
>+

Re: [PATCH] Staging: wlan-ng: hfa384x_usb: Fixed sparse warning cast to restricted __le16

[Frans Klaver]

On Fri, Jun 16, 2017 at 12:05 PM, Jaya Durga  wrote:
> when building with make C=1 CF=-D__CHECK_ENDIAN__
>
> drivers/staging/wlan-ng/hfa384x_usb.c:3383:36: warning: cast to restricted 
> __le16
>
> fixed by using the le16_to_cpus function.
>
> Signed-off-by: Jaya Durga 
> ---
>  drivers/staging/wlan-ng/hfa384x_usb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/wlan-ng/hfa384x_usb.c 
> b/drivers/staging/wlan-ng/hfa384x_usb.c
> index a812e55..0e95e45 100644
> --- a/drivers/staging/wlan-ng/hfa384x_usb.c
> +++ b/drivers/staging/wlan-ng/hfa384x_usb.c
> @@ -3380,7 +3380,7 @@ static void hfa384x_usbin_rx(struct wlandevice 
> *wlandev, struct sk_buff *skb)
> u16 fc;
>
> /* Byte order convert once up front. */
> -   usbin->rxfrm.desc.status = le16_to_cpu(usbin->rxfrm.desc.status);
> +   le16_to_cpus(>rxfrm.desc.status);

Why is a cpu ordered value stored into a field that is documented to
be little endian? When this happens we can't rely on this variable
being little endian, can we? With the way things are now, at least
it's flagged by some tool. Come this patch, it won't be.

I don't think this patch fixes anything; it just tells sparse to shut up.

Frans

Re: [PATCH] Staging: wlan-ng: hfa384x_usb: Fixed sparse warning cast to restricted __le16

[Frans Klaver]

On Fri, Jun 16, 2017 at 12:05 PM, Jaya Durga  wrote:
> when building with make C=1 CF=-D__CHECK_ENDIAN__
>
> drivers/staging/wlan-ng/hfa384x_usb.c:3383:36: warning: cast to restricted 
> __le16
>
> fixed by using the le16_to_cpus function.
>
> Signed-off-by: Jaya Durga 
> ---
>  drivers/staging/wlan-ng/hfa384x_usb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/wlan-ng/hfa384x_usb.c 
> b/drivers/staging/wlan-ng/hfa384x_usb.c
> index a812e55..0e95e45 100644
> --- a/drivers/staging/wlan-ng/hfa384x_usb.c
> +++ b/drivers/staging/wlan-ng/hfa384x_usb.c
> @@ -3380,7 +3380,7 @@ static void hfa384x_usbin_rx(struct wlandevice 
> *wlandev, struct sk_buff *skb)
> u16 fc;
>
> /* Byte order convert once up front. */
> -   usbin->rxfrm.desc.status = le16_to_cpu(usbin->rxfrm.desc.status);
> +   le16_to_cpus(>rxfrm.desc.status);

Why is a cpu ordered value stored into a field that is documented to
be little endian? When this happens we can't rely on this variable
being little endian, can we? With the way things are now, at least
it's flagged by some tool. Come this patch, it won't be.

I don't think this patch fixes anything; it just tells sparse to shut up.

Frans

Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]

[Frans Klaver]

Hm. For some reason the great mail filtering scheme decided to push
this past my inbox :-/

On Sat, Jun 17, 2017 at 12:44 AM, Joe Perches <j...@perches.com> wrote:
> On Fri, 2017-06-16 at 19:45 +0200, Frans Klaver wrote:
>> The header field in struct pd_message is declared as an __le16 type. The
>> data in the message is supposed to be little endian. This means we don't
>> have to go and shift the individual bytes into position when we're
>> filling the buffer, we can just copy the contents right away. As an
>> added benefit we don't get fishy results on big endian systems anymore.
>
> Thanks for pointing this out.
>
> There are several instances of this class of error.

There are other smells around __(le|be) types that show up in staging
that might be worth checking in the rest of the kernel as well. e.g.
converting to cpu and storing it back into itself (possibly with its
bytes reversed), direct assignments without conversion and what else
you might have. sparse obviously already flags anything fishy going on
with these types, but cannot distinguish between the classes of
errors. I'll need to acquaint myself with spatch a bit more to be able
to track that down.

Thanks,
Frans

Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]

[Frans Klaver]

Hm. For some reason the great mail filtering scheme decided to push
this past my inbox :-/

On Sat, Jun 17, 2017 at 12:44 AM, Joe Perches  wrote:
> On Fri, 2017-06-16 at 19:45 +0200, Frans Klaver wrote:
>> The header field in struct pd_message is declared as an __le16 type. The
>> data in the message is supposed to be little endian. This means we don't
>> have to go and shift the individual bytes into position when we're
>> filling the buffer, we can just copy the contents right away. As an
>> added benefit we don't get fishy results on big endian systems anymore.
>
> Thanks for pointing this out.
>
> There are several instances of this class of error.

There are other smells around __(le|be) types that show up in staging
that might be worth checking in the rest of the kernel as well. e.g.
converting to cpu and storing it back into itself (possibly with its
bytes reversed), direct assignments without conversion and what else
you might have. sparse obviously already flags anything fishy going on
with these types, but cannot distinguish between the classes of
errors. I'll need to acquaint myself with spatch a bit more to be able
to track that down.

Thanks,
Frans

[PATCH] staging: fusb302: don't bitshift __le16 type

[Frans Klaver]

The header field in struct pd_message is declared as an __le16 type. The
data in the message is supposed to be little endian. This means we don't
have to go and shift the individual bytes into position when we're
filling the buffer, we can just copy the contents right away. As an
added benefit we don't get fishy results on big endian systems anymore.

Signed-off-by: Frans Klaver <franskla...@gmail.com>
---
 drivers/staging/typec/fusb302/fusb302.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/typec/fusb302/fusb302.c 
b/drivers/staging/typec/fusb302/fusb302.c
index 4a356e509fe4..03a3809d18f0 100644
--- a/drivers/staging/typec/fusb302/fusb302.c
+++ b/drivers/staging/typec/fusb302/fusb302.c
@@ -1039,8 +1039,8 @@ static int fusb302_pd_send_message(struct fusb302_chip 
*chip,
}
/* packsym tells the FUSB302 chip that the next X bytes are payload */
buf[pos++] = FUSB302_TKN_PACKSYM | (len & 0x1F);
-   buf[pos++] = msg->header & 0xFF;
-   buf[pos++] = (msg->header >> 8) & 0xFF;
+   memcpy([pos], >header, sizeof(msg->header));
+   pos += sizeof(msg->header);
 
len -= 2;
memcpy([pos], msg->payload, len);
-- 
2.13.0

[PATCH] staging: fusb302: don't bitshift __le16 type

[Frans Klaver]

The header field in struct pd_message is declared as an __le16 type. The
data in the message is supposed to be little endian. This means we don't
have to go and shift the individual bytes into position when we're
filling the buffer, we can just copy the contents right away. As an
added benefit we don't get fishy results on big endian systems anymore.

Signed-off-by: Frans Klaver 
---
 drivers/staging/typec/fusb302/fusb302.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/typec/fusb302/fusb302.c 
b/drivers/staging/typec/fusb302/fusb302.c
index 4a356e509fe4..03a3809d18f0 100644
--- a/drivers/staging/typec/fusb302/fusb302.c
+++ b/drivers/staging/typec/fusb302/fusb302.c
@@ -1039,8 +1039,8 @@ static int fusb302_pd_send_message(struct fusb302_chip 
*chip,
}
/* packsym tells the FUSB302 chip that the next X bytes are payload */
buf[pos++] = FUSB302_TKN_PACKSYM | (len & 0x1F);
-   buf[pos++] = msg->header & 0xFF;
-   buf[pos++] = (msg->header >> 8) & 0xFF;
+   memcpy([pos], >header, sizeof(msg->header));
+   pos += sizeof(msg->header);
 
len -= 2;
memcpy([pos], msg->payload, len);
-- 
2.13.0

Re: [PATCH v2] staging: wlan-ng: Amend type mismatch warnings

[Frans Klaver]

> Subject: [PATCH v2] staging: wlan-ng: Amend type mismatch warnings

I think it would be better to state that you fix the types of some
struct fields. That's a much more important goal of this patch than
getting sparse to spout slightly fewer warnings.

On Thu, Jun 15, 2017 at 8:41 AM,  <suni...@techveda.org> wrote:
> From: Suniel Mahesh <suni...@techveda.org>
>
> le16_to_cpu() accepts argument of type __le16 and cpu_to_le16()
> returns an argument of type __le16. This patch fixes warnings
> related to incorrect type in assignment and changes the types
> in the corresponding header file.
> The following type mismatch warnings reported by sparse
> have been amended:

You didn't amend them; you removed them.

> warning: cast to restricted __le16
> warning: incorrect type in assignment (different base types)
>
> Signed-off-by: Suniel Mahesh <suni...@techveda.org>
> ---
> Changes for v2:
> - Reworked on the patch and modified commit message as per the
>   recommendations from Frans Klaver and Greg K-H.
>
> - Patch was tested and built on next-20170609 and staging-testing.
> ---
>  drivers/staging/wlan-ng/hfa384x.h| 18 +-
>  drivers/staging/wlan-ng/prism2mgmt.c |  2 +-
>  2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/wlan-ng/hfa384x.h 
> b/drivers/staging/wlan-ng/hfa384x.h
> index 310e2c4..f99cc04 100644
> --- a/drivers/staging/wlan-ng/hfa384x.h
> +++ b/drivers/staging/wlan-ng/hfa384x.h
> @@ -358,7 +358,7 @@ struct hfa384x_bytestr {
>  } __packed;
>
>  struct hfa384x_bytestr32 {
> -   u16 len;
> +   __le16 len;
> u8 data[32];
>  } __packed;
>
> @@ -399,8 +399,8 @@ struct hfa384x_caplevel {
>
>  /*-- Configuration Record: HostScanRequest (data portion only) --*/
>  struct hfa384x_host_scan_request_data {
> -   u16 channel_list;
> -   u16 tx_rate;
> +   __le16 channel_list;
> +   __le16 tx_rate;
> struct hfa384x_bytestr32 ssid;
>  } __packed;
>
> @@ -682,16 +682,16 @@ struct hfa384x_ch_info_result {
>
>  /*--  Inquiry Frame, Diagnose: Host Scan Results & Subfields--*/
>  struct hfa384x_hscan_result_sub {
> -   u16 chid;
> -   u16 anl;
> -   u16 sl;
> +   __le16 chid;
> +   __le16 anl;
> +   __le16 sl;
> u8 bssid[WLAN_BSSID_LEN];
> -   u16 bcnint;
> -   u16 capinfo;
> +   __le16 bcnint;
> +   __le16 capinfo;
> struct hfa384x_bytestr32 ssid;
> u8 supprates[10];   /* 802.11 info element */
> u16 proberesp_rate;
> -   u16 atim;
> +   __le16 atim;
>  } __packed;
>
>  struct hfa384x_hscan_result {
> diff --git a/drivers/staging/wlan-ng/prism2mgmt.c 
> b/drivers/staging/wlan-ng/prism2mgmt.c
> index f4d6e48..c4aa9e7 100644
> --- a/drivers/staging/wlan-ng/prism2mgmt.c
> +++ b/drivers/staging/wlan-ng/prism2mgmt.c
> @@ -213,7 +213,7 @@ int prism2mgmt_scan(struct wlandevice *wlandev, void 
> *msgp)
> goto exit;
> }
> if (word == HFA384x_PORTSTATUS_DISABLED) {
> -   u16 wordbuf[17];
> +   __le16 wordbuf[17];
>
> result = hfa384x_drvr_setconfig16(hw,
> HFA384x_RID_CNFROAMINGMODE,
> --
> 1.9.1
>

Cheers,
Frans

Re: [PATCH v2] staging: wlan-ng: Amend type mismatch warnings

[Frans Klaver]

> Subject: [PATCH v2] staging: wlan-ng: Amend type mismatch warnings

I think it would be better to state that you fix the types of some
struct fields. That's a much more important goal of this patch than
getting sparse to spout slightly fewer warnings.

On Thu, Jun 15, 2017 at 8:41 AM,   wrote:
> From: Suniel Mahesh 
>
> le16_to_cpu() accepts argument of type __le16 and cpu_to_le16()
> returns an argument of type __le16. This patch fixes warnings
> related to incorrect type in assignment and changes the types
> in the corresponding header file.
> The following type mismatch warnings reported by sparse
> have been amended:

You didn't amend them; you removed them.

> warning: cast to restricted __le16
> warning: incorrect type in assignment (different base types)
>
> Signed-off-by: Suniel Mahesh 
> ---
> Changes for v2:
> - Reworked on the patch and modified commit message as per the
>   recommendations from Frans Klaver and Greg K-H.
>
> - Patch was tested and built on next-20170609 and staging-testing.
> ---
>  drivers/staging/wlan-ng/hfa384x.h| 18 +-
>  drivers/staging/wlan-ng/prism2mgmt.c |  2 +-
>  2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/wlan-ng/hfa384x.h 
> b/drivers/staging/wlan-ng/hfa384x.h
> index 310e2c4..f99cc04 100644
> --- a/drivers/staging/wlan-ng/hfa384x.h
> +++ b/drivers/staging/wlan-ng/hfa384x.h
> @@ -358,7 +358,7 @@ struct hfa384x_bytestr {
>  } __packed;
>
>  struct hfa384x_bytestr32 {
> -   u16 len;
> +   __le16 len;
> u8 data[32];
>  } __packed;
>
> @@ -399,8 +399,8 @@ struct hfa384x_caplevel {
>
>  /*-- Configuration Record: HostScanRequest (data portion only) --*/
>  struct hfa384x_host_scan_request_data {
> -   u16 channel_list;
> -   u16 tx_rate;
> +   __le16 channel_list;
> +   __le16 tx_rate;
> struct hfa384x_bytestr32 ssid;
>  } __packed;
>
> @@ -682,16 +682,16 @@ struct hfa384x_ch_info_result {
>
>  /*--  Inquiry Frame, Diagnose: Host Scan Results & Subfields--*/
>  struct hfa384x_hscan_result_sub {
> -   u16 chid;
> -   u16 anl;
> -   u16 sl;
> +   __le16 chid;
> +   __le16 anl;
> +   __le16 sl;
> u8 bssid[WLAN_BSSID_LEN];
> -   u16 bcnint;
> -   u16 capinfo;
> +   __le16 bcnint;
> +   __le16 capinfo;
> struct hfa384x_bytestr32 ssid;
> u8 supprates[10];   /* 802.11 info element */
> u16 proberesp_rate;
> -   u16 atim;
> +   __le16 atim;
>  } __packed;
>
>  struct hfa384x_hscan_result {
> diff --git a/drivers/staging/wlan-ng/prism2mgmt.c 
> b/drivers/staging/wlan-ng/prism2mgmt.c
> index f4d6e48..c4aa9e7 100644
> --- a/drivers/staging/wlan-ng/prism2mgmt.c
> +++ b/drivers/staging/wlan-ng/prism2mgmt.c
> @@ -213,7 +213,7 @@ int prism2mgmt_scan(struct wlandevice *wlandev, void 
> *msgp)
> goto exit;
> }
> if (word == HFA384x_PORTSTATUS_DISABLED) {
> -   u16 wordbuf[17];
> +   __le16 wordbuf[17];
>
> result = hfa384x_drvr_setconfig16(hw,
> HFA384x_RID_CNFROAMINGMODE,
> --
> 1.9.1
>

Cheers,
Frans

Re: [PATCH] staging: wlan-ng: Amend type mismatch warnings

[Frans Klaver]

On Mon, Jun 12, 2017 at 7:15 PM,   wrote:
> From: Suniel Mahesh 
>
> The following type mismatch warnings reported by sparse
> have been amended:
> warning: cast to restricted __le16
> warning: incorrect type in assignment (different base types)

Why not change the type of the struct fields to __le16 where they
would need to be __le16 (thereby documenting the requirement)? This is
just telling the compiler to shut up, not necessarily fixing the issue
that it's flagging.

Cheers,
Frans


>
> Signed-off-by: Suniel Mahesh 
> ---
>  drivers/staging/wlan-ng/prism2mgmt.c | 22 +++---
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/staging/wlan-ng/prism2mgmt.c 
> b/drivers/staging/wlan-ng/prism2mgmt.c
> index f4d6e48..358b556 100644
> --- a/drivers/staging/wlan-ng/prism2mgmt.c
> +++ b/drivers/staging/wlan-ng/prism2mgmt.c
> @@ -185,7 +185,7 @@ int prism2mgmt_scan(struct wlandevice *wlandev, void 
> *msgp)
>
> /* set up the txrate to be 2MBPS. Should be fastest basicrate... */
> word = HFA384x_RATEBIT_2;
> -   scanreq.tx_rate = cpu_to_le16(word);
> +   scanreq.tx_rate = (__force u16)cpu_to_le16(word);
>
> /* set up the channel list */
> word = 0;
> @@ -197,10 +197,10 @@ int prism2mgmt_scan(struct wlandevice *wlandev, void 
> *msgp)
> /* channel 1 is BIT 0 ... channel 14 is BIT 13 */
> word |= (1 << (channel - 1));
> }
> -   scanreq.channel_list = cpu_to_le16(word);
> +   scanreq.channel_list = (__force u16)cpu_to_le16(word);
>
> /* set up the ssid, if present. */
> -   scanreq.ssid.len = cpu_to_le16(msg->ssid.data.len);
> +   scanreq.ssid.len = (__force u16)cpu_to_le16(msg->ssid.data.len);
> memcpy(scanreq.ssid.data, msg->ssid.data.data, msg->ssid.data.len);
>
> /* Enable the MAC port if it's not already enabled  */
> @@ -229,7 +229,7 @@ int prism2mgmt_scan(struct wlandevice *wlandev, void 
> *msgp)
> /* Construct a bogus SSID and assign it to OwnSSID and
>  * DesiredSSID
>  */
> -   wordbuf[0] = cpu_to_le16(WLAN_SSID_MAXLEN);
> +   wordbuf[0] = (__force u16)cpu_to_le16(WLAN_SSID_MAXLEN);
> get_random_bytes([1], WLAN_SSID_MAXLEN);
> result = hfa384x_drvr_setconfig(hw, HFA384x_RID_CNFOWNSSID,
> wordbuf,
> @@ -405,8 +405,8 @@ int prism2mgmt_scan_results(struct wlandevice *wlandev, 
> void *msgp)
> /* signal and noise */
> req->signal.status = P80211ENUM_msgitem_status_data_ok;
> req->noise.status = P80211ENUM_msgitem_status_data_ok;
> -   req->signal.data = le16_to_cpu(item->sl);
> -   req->noise.data = le16_to_cpu(item->anl);
> +   req->signal.data = le16_to_cpu((__force __le16)item->sl);
> +   req->noise.data = le16_to_cpu((__force __le16)item->anl);
>
> /* BSSID */
> req->bssid.status = P80211ENUM_msgitem_status_data_ok;
> @@ -415,7 +415,7 @@ int prism2mgmt_scan_results(struct wlandevice *wlandev, 
> void *msgp)
>
> /* SSID */
> req->ssid.status = P80211ENUM_msgitem_status_data_ok;
> -   req->ssid.data.len = le16_to_cpu(item->ssid.len);
> +   req->ssid.data.len = le16_to_cpu((__force __le16)item->ssid.len);
> req->ssid.data.len = min_t(u16, req->ssid.data.len, WLAN_SSID_MAXLEN);
> memcpy(req->ssid.data.data, item->ssid.data, req->ssid.data.len);
>
> @@ -463,7 +463,7 @@ int prism2mgmt_scan_results(struct wlandevice *wlandev, 
> void *msgp)
>
> /* beacon period */
> req->beaconperiod.status = P80211ENUM_msgitem_status_data_ok;
> -   req->beaconperiod.data = le16_to_cpu(item->bcnint);
> +   req->beaconperiod.data = le16_to_cpu((__force __le16)item->bcnint);
>
> /* timestamps */
> req->timestamp.status = P80211ENUM_msgitem_status_data_ok;
> @@ -473,14 +473,14 @@ int prism2mgmt_scan_results(struct wlandevice *wlandev, 
> void *msgp)
>
> /* atim window */
> req->ibssatimwindow.status = P80211ENUM_msgitem_status_data_ok;
> -   req->ibssatimwindow.data = le16_to_cpu(item->atim);
> +   req->ibssatimwindow.data = le16_to_cpu((__force __le16)item->atim);
>
> /* Channel */
> req->dschannel.status = P80211ENUM_msgitem_status_data_ok;
> -   req->dschannel.data = le16_to_cpu(item->chid);
> +   req->dschannel.data = le16_to_cpu((__force __le16)item->chid);
>
> /* capinfo bits */
> -   count = le16_to_cpu(item->capinfo);
> +   count = le16_to_cpu((__force __le16)item->capinfo);
> req->capinfo.status = P80211ENUM_msgitem_status_data_ok;
> req->capinfo.data = count;
>
> --
> 1.9.1
>

Re: [PATCH] staging: wlan-ng: Amend type mismatch warnings

[Frans Klaver]

On Mon, Jun 12, 2017 at 7:15 PM,   wrote:
> From: Suniel Mahesh 
>
> The following type mismatch warnings reported by sparse
> have been amended:
> warning: cast to restricted __le16
> warning: incorrect type in assignment (different base types)

Why not change the type of the struct fields to __le16 where they
would need to be __le16 (thereby documenting the requirement)? This is
just telling the compiler to shut up, not necessarily fixing the issue
that it's flagging.

Cheers,
Frans


>
> Signed-off-by: Suniel Mahesh 
> ---
>  drivers/staging/wlan-ng/prism2mgmt.c | 22 +++---
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/staging/wlan-ng/prism2mgmt.c 
> b/drivers/staging/wlan-ng/prism2mgmt.c
> index f4d6e48..358b556 100644
> --- a/drivers/staging/wlan-ng/prism2mgmt.c
> +++ b/drivers/staging/wlan-ng/prism2mgmt.c
> @@ -185,7 +185,7 @@ int prism2mgmt_scan(struct wlandevice *wlandev, void 
> *msgp)
>
> /* set up the txrate to be 2MBPS. Should be fastest basicrate... */
> word = HFA384x_RATEBIT_2;
> -   scanreq.tx_rate = cpu_to_le16(word);
> +   scanreq.tx_rate = (__force u16)cpu_to_le16(word);
>
> /* set up the channel list */
> word = 0;
> @@ -197,10 +197,10 @@ int prism2mgmt_scan(struct wlandevice *wlandev, void 
> *msgp)
> /* channel 1 is BIT 0 ... channel 14 is BIT 13 */
> word |= (1 << (channel - 1));
> }
> -   scanreq.channel_list = cpu_to_le16(word);
> +   scanreq.channel_list = (__force u16)cpu_to_le16(word);
>
> /* set up the ssid, if present. */
> -   scanreq.ssid.len = cpu_to_le16(msg->ssid.data.len);
> +   scanreq.ssid.len = (__force u16)cpu_to_le16(msg->ssid.data.len);
> memcpy(scanreq.ssid.data, msg->ssid.data.data, msg->ssid.data.len);
>
> /* Enable the MAC port if it's not already enabled  */
> @@ -229,7 +229,7 @@ int prism2mgmt_scan(struct wlandevice *wlandev, void 
> *msgp)
> /* Construct a bogus SSID and assign it to OwnSSID and
>  * DesiredSSID
>  */
> -   wordbuf[0] = cpu_to_le16(WLAN_SSID_MAXLEN);
> +   wordbuf[0] = (__force u16)cpu_to_le16(WLAN_SSID_MAXLEN);
> get_random_bytes([1], WLAN_SSID_MAXLEN);
> result = hfa384x_drvr_setconfig(hw, HFA384x_RID_CNFOWNSSID,
> wordbuf,
> @@ -405,8 +405,8 @@ int prism2mgmt_scan_results(struct wlandevice *wlandev, 
> void *msgp)
> /* signal and noise */
> req->signal.status = P80211ENUM_msgitem_status_data_ok;
> req->noise.status = P80211ENUM_msgitem_status_data_ok;
> -   req->signal.data = le16_to_cpu(item->sl);
> -   req->noise.data = le16_to_cpu(item->anl);
> +   req->signal.data = le16_to_cpu((__force __le16)item->sl);
> +   req->noise.data = le16_to_cpu((__force __le16)item->anl);
>
> /* BSSID */
> req->bssid.status = P80211ENUM_msgitem_status_data_ok;
> @@ -415,7 +415,7 @@ int prism2mgmt_scan_results(struct wlandevice *wlandev, 
> void *msgp)
>
> /* SSID */
> req->ssid.status = P80211ENUM_msgitem_status_data_ok;
> -   req->ssid.data.len = le16_to_cpu(item->ssid.len);
> +   req->ssid.data.len = le16_to_cpu((__force __le16)item->ssid.len);
> req->ssid.data.len = min_t(u16, req->ssid.data.len, WLAN_SSID_MAXLEN);
> memcpy(req->ssid.data.data, item->ssid.data, req->ssid.data.len);
>
> @@ -463,7 +463,7 @@ int prism2mgmt_scan_results(struct wlandevice *wlandev, 
> void *msgp)
>
> /* beacon period */
> req->beaconperiod.status = P80211ENUM_msgitem_status_data_ok;
> -   req->beaconperiod.data = le16_to_cpu(item->bcnint);
> +   req->beaconperiod.data = le16_to_cpu((__force __le16)item->bcnint);
>
> /* timestamps */
> req->timestamp.status = P80211ENUM_msgitem_status_data_ok;
> @@ -473,14 +473,14 @@ int prism2mgmt_scan_results(struct wlandevice *wlandev, 
> void *msgp)
>
> /* atim window */
> req->ibssatimwindow.status = P80211ENUM_msgitem_status_data_ok;
> -   req->ibssatimwindow.data = le16_to_cpu(item->atim);
> +   req->ibssatimwindow.data = le16_to_cpu((__force __le16)item->atim);
>
> /* Channel */
> req->dschannel.status = P80211ENUM_msgitem_status_data_ok;
> -   req->dschannel.data = le16_to_cpu(item->chid);
> +   req->dschannel.data = le16_to_cpu((__force __le16)item->chid);
>
> /* capinfo bits */
> -   count = le16_to_cpu(item->capinfo);
> +   count = le16_to_cpu((__force __le16)item->capinfo);
> req->capinfo.status = P80211ENUM_msgitem_status_data_ok;
> req->capinfo.data = count;
>
> --
> 1.9.1
>

Re: [PATCH v2] staging: goldfish: Fix style issues in macros

[Frans Klaver]

On Mon, Mar 27, 2017 at 2:13 PM, aviyae <aviya...@gmail.com> wrote:
> I'll remove that eudyptula thing, although there are some patches that 
> mention it
>
> (run git log --grep=eudyptula)

I haven't checked that and I think it's a shame it didn't get removed.
I don't think it adds value to the patch, as it's just for a personal
goal.

It is also custom to reply _below_ the (part of the) message you're
responding to by the way ;-).

Cheers,
Frans


> On 27/03/17 15:05, Frans Klaver wrote:
>> On Mon, Mar 27, 2017 at 1:52 PM, aviyae <aviya...@gmail.com> wrote:
>>> From: Aviya Erenfeld <aviya...@gmail.com>
>>>
>>> staging: goldfish: Fix style issues in macros
>>>
>>> Fix coding style issues in macros:
>>> 1. Add parenthesis around macros argument
>>> 2. Avoid arguments reuse in macros
>>>
>>> (For the eudyptula challenge)
>> How is that relevant?
>>
>> Frans
>

Re: [PATCH v2] staging: goldfish: Fix style issues in macros

[Frans Klaver]

On Mon, Mar 27, 2017 at 2:13 PM, aviyae  wrote:
> I'll remove that eudyptula thing, although there are some patches that 
> mention it
>
> (run git log --grep=eudyptula)

I haven't checked that and I think it's a shame it didn't get removed.
I don't think it adds value to the patch, as it's just for a personal
goal.

It is also custom to reply _below_ the (part of the) message you're
responding to by the way ;-).

Cheers,
Frans


> On 27/03/17 15:05, Frans Klaver wrote:
>> On Mon, Mar 27, 2017 at 1:52 PM, aviyae  wrote:
>>> From: Aviya Erenfeld 
>>>
>>> staging: goldfish: Fix style issues in macros
>>>
>>> Fix coding style issues in macros:
>>> 1. Add parenthesis around macros argument
>>> 2. Avoid arguments reuse in macros
>>>
>>> (For the eudyptula challenge)
>> How is that relevant?
>>
>> Frans
>

Re: [PATCH v2] staging: goldfish: Fix style issues in macros

[Frans Klaver]

On Mon, Mar 27, 2017 at 1:52 PM, aviyae  wrote:
> From: Aviya Erenfeld 
>
> staging: goldfish: Fix style issues in macros
>
> Fix coding style issues in macros:
> 1. Add parenthesis around macros argument
> 2. Avoid arguments reuse in macros
>
> (For the eudyptula challenge)

How is that relevant?

Frans

Re: [PATCH v2] staging: goldfish: Fix style issues in macros

[Frans Klaver]

On Mon, Mar 27, 2017 at 1:52 PM, aviyae  wrote:
> From: Aviya Erenfeld 
>
> staging: goldfish: Fix style issues in macros
>
> Fix coding style issues in macros:
> 1. Add parenthesis around macros argument
> 2. Avoid arguments reuse in macros
>
> (For the eudyptula challenge)

How is that relevant?

Frans

Re: [PATCH] drm/i915/kvmgt: avoid dereferencing a potentially null info pointer

[Frans Klaver]

On Thu, Mar 23, 2017 at 1:22 PM, Colin King  wrote:
> From: Colin Ian King 
>
> info is being checked to see if it is a null pointer, however, vpgu is
> dereferencing info before this check, leading to a potential null
> pointer dereference.  If info is null, then the error message being
> printed by macro gvt_vgpu_err and this requires vpgu to exist. We can
> use a null vpgu as the macro has a sanity check to see if vpgu is null,
> so this is OK.
>
> Detected with CoverityScan, CID#1420672 ("Dereference nefore null check")

s,nefore,before,


>
> Fixes: 695fbc08d80f ("drm/i915/gvt: replace the gvt_err with gvt_vgpu_err")
> Signed-off-by: Colin Ian King 
> ---
>  drivers/gpu/drm/i915/gvt/kvmgt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c 
> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 1ea3eb270de8..f8619a772c44 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -1339,9 +1339,9 @@ static int kvmgt_guest_init(struct mdev_device *mdev)
>
>  static bool kvmgt_guest_exit(struct kvmgt_guest_info *info)
>  {
> -   struct intel_vgpu *vgpu = info->vgpu;
> -
> if (!info) {
> +   struct intel_vgpu *vgpu = NULL;
> +
> gvt_vgpu_err("kvmgt_guest_info invalid\n");
> return false;
> }

Does this mean the original gvt_err() macro is no longer there?

And apparently gvt_vgpu_err is a macro that depends on the specifics
of its environment? Yikes.

Cheers,
Frans

Re: [PATCH] drm/i915/kvmgt: avoid dereferencing a potentially null info pointer

[Frans Klaver]

On Thu, Mar 23, 2017 at 1:22 PM, Colin King  wrote:
> From: Colin Ian King 
>
> info is being checked to see if it is a null pointer, however, vpgu is
> dereferencing info before this check, leading to a potential null
> pointer dereference.  If info is null, then the error message being
> printed by macro gvt_vgpu_err and this requires vpgu to exist. We can
> use a null vpgu as the macro has a sanity check to see if vpgu is null,
> so this is OK.
>
> Detected with CoverityScan, CID#1420672 ("Dereference nefore null check")

s,nefore,before,


>
> Fixes: 695fbc08d80f ("drm/i915/gvt: replace the gvt_err with gvt_vgpu_err")
> Signed-off-by: Colin Ian King 
> ---
>  drivers/gpu/drm/i915/gvt/kvmgt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c 
> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 1ea3eb270de8..f8619a772c44 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -1339,9 +1339,9 @@ static int kvmgt_guest_init(struct mdev_device *mdev)
>
>  static bool kvmgt_guest_exit(struct kvmgt_guest_info *info)
>  {
> -   struct intel_vgpu *vgpu = info->vgpu;
> -
> if (!info) {
> +   struct intel_vgpu *vgpu = NULL;
> +
> gvt_vgpu_err("kvmgt_guest_info invalid\n");
> return false;
> }

Does this mean the original gvt_err() macro is no longer there?

And apparently gvt_vgpu_err is a macro that depends on the specifics
of its environment? Yikes.

Cheers,
Frans

[PATCH] staging: wlan_ng: fix logical continuation alignment

[Frans Klaver]

It appears that our coding style prefers that logical continuations
have the operator at the end of the line. Fix that.

While at it, stick the 'if' after 'else' where it belongs.

Signed-off-by: Frans Klaver <franskla...@gmail.com>
---
 drivers/staging/wlan-ng/prism2mgmt.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/wlan-ng/prism2mgmt.c 
b/drivers/staging/wlan-ng/prism2mgmt.c
index c558ad6..0e671c3 100644
--- a/drivers/staging/wlan-ng/prism2mgmt.c
+++ b/drivers/staging/wlan-ng/prism2mgmt.c
@@ -1303,14 +1303,13 @@ int prism2mgmt_wlansniff(struct wlandevice *wlandev, 
void *msgp)
/* Set the driver state */
/* Do we want the prism2 header? */
if ((msg->prismheader.status ==
-P80211ENUM_msgitem_status_data_ok)
-   && (msg->prismheader.data == P80211ENUM_truth_true)) {
+P80211ENUM_msgitem_status_data_ok) &&
+   (msg->prismheader.data == P80211ENUM_truth_true)) {
hw->sniffhdr = 0;
wlandev->netdev->type = ARPHRD_IEEE80211_PRISM;
-   } else
-   if ((msg->wlanheader.status ==
-P80211ENUM_msgitem_status_data_ok)
-   && (msg->wlanheader.data == P80211ENUM_truth_true)) {
+   } else if ((msg->wlanheader.status ==
+   P80211ENUM_msgitem_status_data_ok) &&
+  (msg->wlanheader.data == P80211ENUM_truth_true)) {
hw->sniffhdr = 1;
wlandev->netdev->type = ARPHRD_IEEE80211_PRISM;
} else {
-- 
2.10.2

[PATCH] staging: wlan_ng: fix logical continuation alignment

[Frans Klaver]

It appears that our coding style prefers that logical continuations
have the operator at the end of the line. Fix that.

While at it, stick the 'if' after 'else' where it belongs.

Signed-off-by: Frans Klaver 
---
 drivers/staging/wlan-ng/prism2mgmt.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/wlan-ng/prism2mgmt.c 
b/drivers/staging/wlan-ng/prism2mgmt.c
index c558ad6..0e671c3 100644
--- a/drivers/staging/wlan-ng/prism2mgmt.c
+++ b/drivers/staging/wlan-ng/prism2mgmt.c
@@ -1303,14 +1303,13 @@ int prism2mgmt_wlansniff(struct wlandevice *wlandev, 
void *msgp)
/* Set the driver state */
/* Do we want the prism2 header? */
if ((msg->prismheader.status ==
-P80211ENUM_msgitem_status_data_ok)
-   && (msg->prismheader.data == P80211ENUM_truth_true)) {
+P80211ENUM_msgitem_status_data_ok) &&
+   (msg->prismheader.data == P80211ENUM_truth_true)) {
hw->sniffhdr = 0;
wlandev->netdev->type = ARPHRD_IEEE80211_PRISM;
-   } else
-   if ((msg->wlanheader.status ==
-P80211ENUM_msgitem_status_data_ok)
-   && (msg->wlanheader.data == P80211ENUM_truth_true)) {
+   } else if ((msg->wlanheader.status ==
+   P80211ENUM_msgitem_status_data_ok) &&
+  (msg->wlanheader.data == P80211ENUM_truth_true)) {
hw->sniffhdr = 1;
wlandev->netdev->type = ARPHRD_IEEE80211_PRISM;
} else {
-- 
2.10.2

Re: [PATCH v05 27/72] linux/if.h linux/hdlc/ioctl.h: move IFNAMSIZ definition to hdlc/ioctl.h

[Frans Klaver]

On Tue, Aug 23, 2016 at 10:03 AM, Frans Klaver <franskla...@gmail.com> wrote:
> On Tue, Aug 23, 2016 at 9:05 AM, David Miller <da...@davemloft.net> wrote:
>> From: Frans Klaver <franskla...@gmail.com>
>> Date: Tue, 23 Aug 2016 09:03:20 +0200
>>
>>> On Tue, Aug 23, 2016 at 1:30 AM, David Miller <da...@davemloft.net> wrote:
>>>> From: Mikko Rapeli <mikko.rap...@iki.fi>
>>>> Date: Mon, 22 Aug 2016 20:32:44 +0200
>>>>
>>>>> Fixes userspace compiler error:
>>>>>
>>>>> error: ‘IFNAMSIZ’ undeclared here (not in a function)
>>>>>
>>>>> Suggested by Frans Klaver <franskla...@gmail.com> on lkml message
>>>>> <20150530195223.ga15...@bugger.home>.
>>>>>
>>>>> Signed-off-by: Mikko Rapeli <mikko.rap...@iki.fi>
>>>>
>>>> IFNAMSIZ has to be in linux/if.h, you aren't explaining why you have
>>>> to move it to the hdlc header instead of having the hdlc header
>>>> include linux/if.h
>>>
>>> Circular references. linux/if.h includes hdlc/ioctl.h, and has to
>>> define IFNAMSIZ before doing so.
>>
>> That's not acceptable.  Use forward declarations or similar to avoid
>> the circular dependency.
>>
>> IFNAMSIZ belongs in linux/if.h, please keep it there.
>
> I went back to one of the previous patch sets, but couldn't find why
> the circular dependency had to be broken. So if this can be fixed by
> including linux/if.h instead, I'm all for it.

Alright, so the core of the 'problem' is that the structs in
hdlc/ioctl.h are typedefs of anonymous structs, and linux/if.h points
to those types. We can't really forward declare these structs unless
we name them, so the proper approach would be to name them and use
forward declarations in linux/if.h. hdlc/ioctl.h can then include
linux/if.h. linux/if.h should probably keep including hdlc/ioctl.h to
keep depending application builds from breaking.

Re: [PATCH v05 27/72] linux/if.h linux/hdlc/ioctl.h: move IFNAMSIZ definition to hdlc/ioctl.h

[Frans Klaver]

On Tue, Aug 23, 2016 at 10:03 AM, Frans Klaver  wrote:
> On Tue, Aug 23, 2016 at 9:05 AM, David Miller  wrote:
>> From: Frans Klaver 
>> Date: Tue, 23 Aug 2016 09:03:20 +0200
>>
>>> On Tue, Aug 23, 2016 at 1:30 AM, David Miller  wrote:
>>>> From: Mikko Rapeli 
>>>> Date: Mon, 22 Aug 2016 20:32:44 +0200
>>>>
>>>>> Fixes userspace compiler error:
>>>>>
>>>>> error: ‘IFNAMSIZ’ undeclared here (not in a function)
>>>>>
>>>>> Suggested by Frans Klaver  on lkml message
>>>>> <20150530195223.ga15...@bugger.home>.
>>>>>
>>>>> Signed-off-by: Mikko Rapeli 
>>>>
>>>> IFNAMSIZ has to be in linux/if.h, you aren't explaining why you have
>>>> to move it to the hdlc header instead of having the hdlc header
>>>> include linux/if.h
>>>
>>> Circular references. linux/if.h includes hdlc/ioctl.h, and has to
>>> define IFNAMSIZ before doing so.
>>
>> That's not acceptable.  Use forward declarations or similar to avoid
>> the circular dependency.
>>
>> IFNAMSIZ belongs in linux/if.h, please keep it there.
>
> I went back to one of the previous patch sets, but couldn't find why
> the circular dependency had to be broken. So if this can be fixed by
> including linux/if.h instead, I'm all for it.

Alright, so the core of the 'problem' is that the structs in
hdlc/ioctl.h are typedefs of anonymous structs, and linux/if.h points
to those types. We can't really forward declare these structs unless
we name them, so the proper approach would be to name them and use
forward declarations in linux/if.h. hdlc/ioctl.h can then include
linux/if.h. linux/if.h should probably keep including hdlc/ioctl.h to
keep depending application builds from breaking.

Re: [PATCH v05 27/72] linux/if.h linux/hdlc/ioctl.h: move IFNAMSIZ definition to hdlc/ioctl.h

[Frans Klaver]

On Tue, Aug 23, 2016 at 9:05 AM, David Miller <da...@davemloft.net> wrote:
> From: Frans Klaver <franskla...@gmail.com>
> Date: Tue, 23 Aug 2016 09:03:20 +0200
>
>> On Tue, Aug 23, 2016 at 1:30 AM, David Miller <da...@davemloft.net> wrote:
>>> From: Mikko Rapeli <mikko.rap...@iki.fi>
>>> Date: Mon, 22 Aug 2016 20:32:44 +0200
>>>
>>>> Fixes userspace compiler error:
>>>>
>>>> error: ‘IFNAMSIZ’ undeclared here (not in a function)
>>>>
>>>> Suggested by Frans Klaver <franskla...@gmail.com> on lkml message
>>>> <20150530195223.ga15...@bugger.home>.
>>>>
>>>> Signed-off-by: Mikko Rapeli <mikko.rap...@iki.fi>
>>>
>>> IFNAMSIZ has to be in linux/if.h, you aren't explaining why you have
>>> to move it to the hdlc header instead of having the hdlc header
>>> include linux/if.h
>>
>> Circular references. linux/if.h includes hdlc/ioctl.h, and has to
>> define IFNAMSIZ before doing so.
>
> That's not acceptable.  Use forward declarations or similar to avoid
> the circular dependency.
>
> IFNAMSIZ belongs in linux/if.h, please keep it there.

I went back to one of the previous patch sets, but couldn't find why
the circular dependency had to be broken. So if this can be fixed by
including linux/if.h instead, I'm all for it.

Re: [PATCH v05 27/72] linux/if.h linux/hdlc/ioctl.h: move IFNAMSIZ definition to hdlc/ioctl.h

[Frans Klaver]

On Tue, Aug 23, 2016 at 9:05 AM, David Miller  wrote:
> From: Frans Klaver 
> Date: Tue, 23 Aug 2016 09:03:20 +0200
>
>> On Tue, Aug 23, 2016 at 1:30 AM, David Miller  wrote:
>>> From: Mikko Rapeli 
>>> Date: Mon, 22 Aug 2016 20:32:44 +0200
>>>
>>>> Fixes userspace compiler error:
>>>>
>>>> error: ‘IFNAMSIZ’ undeclared here (not in a function)
>>>>
>>>> Suggested by Frans Klaver  on lkml message
>>>> <20150530195223.ga15...@bugger.home>.
>>>>
>>>> Signed-off-by: Mikko Rapeli 
>>>
>>> IFNAMSIZ has to be in linux/if.h, you aren't explaining why you have
>>> to move it to the hdlc header instead of having the hdlc header
>>> include linux/if.h
>>
>> Circular references. linux/if.h includes hdlc/ioctl.h, and has to
>> define IFNAMSIZ before doing so.
>
> That's not acceptable.  Use forward declarations or similar to avoid
> the circular dependency.
>
> IFNAMSIZ belongs in linux/if.h, please keep it there.

I went back to one of the previous patch sets, but couldn't find why
the circular dependency had to be broken. So if this can be fixed by
including linux/if.h instead, I'm all for it.

Re: [PATCH v05 27/72] linux/if.h linux/hdlc/ioctl.h: move IFNAMSIZ definition to hdlc/ioctl.h

[Frans Klaver]

On Tue, Aug 23, 2016 at 1:30 AM, David Miller <da...@davemloft.net> wrote:
> From: Mikko Rapeli <mikko.rap...@iki.fi>
> Date: Mon, 22 Aug 2016 20:32:44 +0200
>
>> Fixes userspace compiler error:
>>
>> error: ‘IFNAMSIZ’ undeclared here (not in a function)
>>
>> Suggested by Frans Klaver <franskla...@gmail.com> on lkml message
>> <20150530195223.ga15...@bugger.home>.
>>
>> Signed-off-by: Mikko Rapeli <mikko.rap...@iki.fi>
>
> IFNAMSIZ has to be in linux/if.h, you aren't explaining why you have
> to move it to the hdlc header instead of having the hdlc header
> include linux/if.h

Circular references. linux/if.h includes hdlc/ioctl.h, and has to
define IFNAMSIZ before doing so. If IFNAMSIZ is moved to hdlc/ioctl.h,
it is still pulled in if one includes linux/if.h. What we gain is that
you can include hdlc/ioctl.h directly (which is what one of the tests
is already doing, iirc).

But yes, it should be explained in the commit message.

Thanks,
Frans

Re: [PATCH v05 27/72] linux/if.h linux/hdlc/ioctl.h: move IFNAMSIZ definition to hdlc/ioctl.h

[Frans Klaver]

On Tue, Aug 23, 2016 at 1:30 AM, David Miller  wrote:
> From: Mikko Rapeli 
> Date: Mon, 22 Aug 2016 20:32:44 +0200
>
>> Fixes userspace compiler error:
>>
>> error: ‘IFNAMSIZ’ undeclared here (not in a function)
>>
>> Suggested by Frans Klaver  on lkml message
>> <20150530195223.ga15...@bugger.home>.
>>
>> Signed-off-by: Mikko Rapeli 
>
> IFNAMSIZ has to be in linux/if.h, you aren't explaining why you have
> to move it to the hdlc header instead of having the hdlc header
> include linux/if.h

Circular references. linux/if.h includes hdlc/ioctl.h, and has to
define IFNAMSIZ before doing so. If IFNAMSIZ is moved to hdlc/ioctl.h,
it is still pulled in if one includes linux/if.h. What we gain is that
you can include hdlc/ioctl.h directly (which is what one of the tests
is already doing, iirc).

But yes, it should be explained in the commit message.

Thanks,
Frans

Re: [PATCH v5 1/2] usb: USB Type-C connector class

[Frans Klaver]

On Wed, Aug 17, 2016 at 3:53 PM, Heikki Krogerus
<heikki.kroge...@linux.intel.com> wrote:
> Hi,
>
> On Wed, Aug 17, 2016 at 03:14:03PM +0200, Frans Klaver wrote:
>> On Wed, Aug 17, 2016 at 12:34 PM, Heikki Krogerus
>> > +static const char * const typec_partner_types[] = {
>> > +   [TYPEC_PARTNER_USB] = "USB",
>> > +   [TYPEC_PARTNER_CHARGER] = "Charger",
>> > +   [TYPEC_PARTNER_ALTMODE] = "Alternate Mode",
>> > +   [TYPEC_PARTNER_ACCESSORY]   = "Accessory",
>> > +};
>> > +
>> > +static ssize_t partner_type_show(struct device *dev,
>> > +struct device_attribute *attr, char *buf)
>> > +{
>> > +   struct typec_partner *partner = container_of(dev, struct 
>> > typec_partner,
>> > +dev);
>> > +
>> > +   return sprintf(buf, "%s\n", typec_partner_types[partner->type]);
>> > +}
>> > +
>> > +static struct device_attribute dev_attr_partner_type = {
>> > +   .attr = {
>> > +   .name = "type",
>> > +   .mode = S_IRUGO,
>> > +   },
>> > +   .show = partner_type_show,
>> > +};
>>
>> Why not use DEVICE_ATTR_RO() for this?
>
> Because I don't want to tie the attribute names to the function names
> in this case. There are other *type* attributes being created in the
> driver, so type_show() is not good, and we can't name the attribute
> "partner_type". The attribute will be placed in group named "partner".
>

Ah, makes sense.

Thanks,
Frans