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] 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 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 &cma_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] 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: [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->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->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->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->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 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->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->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->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->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: [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(&priv->psstatus.status) == PS_SNOOZE) 
> {
> if (cnt_txqbody(priv)) {
> ks_wlan_hw_wakeup_request(priv);
> -   queue_delayed_work(priv->wq, 
> &priv->rw_dwork, 1);
> +   queue_delayed_work(priv->wq,
> +  &priv->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, &card->func->dev);   /* for create sysfs 
> symlinks */
> +   SET_NETDEV_DEV(netdev, &card->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(&pdev->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(&pdev->dev, irq, em_sti_interrupt,
> +   irq = devm_request_irq(&pdev->dev, irq, em_sti_interrupt,
>  IRQF_TIMER | IRQF_IRQPOLL | IRQF_NOBALANCING,
> -dev_name(&pdev->dev), p)) {
> +dev_name(&pdev->dev), p);
> +   if (irq) {
> dev_err(&pdev->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 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] 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 *)&buf, 2);
>>> +   if (ret < 0)
>>> +   return ret;
>>> +   if (ret != 2)
>>> +   return -EIO;
>>> +
>>> +   /* MSB first */
>>> +   return be16_to_cpu(buf);
>>> +}
>>> +
>>> +static ssize_t lt

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 *)&buf, 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[] = {
> +   &sensor_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(&i2c->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: [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(&pci->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 = &chip->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(&spi->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 = &chip->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 = x

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(&dev->device, "visorbus_create_instance failed: %d\n", err);
> +   dev_err(&dev->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  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(&prev->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(&prev->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(&prev->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(&prev->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  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(&dev->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  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(&dev->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(&dev->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] 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(&dev->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(&dev->mlock);
> list_add_tail(&t->list, &dev->targets);
> mutex_unlock(&dev->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  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] 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(&pci->dev, "rtsx_remove() called\n");
>>> + dev_info(&pci->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: 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(&sm750_dev->slock);
> +   spin_lock(&sm750_dev->slock);
>
> sm750_dev->accel.de_fillrect(&sm750_dev->accel,
>  base, pitch, Bpp,
>  region->dx, region->dy,
>  region->width, region->height,
>  color, rop);
> -   if (sm750_dev->fb_count > 1)
> -   spin_unlock(&sm750_dev->slock);
> +   spin_unlock(&sm750_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(&sm750_dev->slock);
> +   spin_lock(&sm750_dev->slock);
>
> sm750_dev->accel.de_copyarea(&sm750_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(&sm750_dev->slock);
> +   spin_unlock(&sm750_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(&sm750_dev->slock);
> +   spin_lock(&sm750_dev->slock);
>
> sm750_dev->accel.de_imageblit(&sm750_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(&sm750_dev->slock);
> +   spin_unlock(&sm750_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: 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(&sm750_dev->slock);
> 
>-  sm750_dev->accel.de_fillrect(&sm750_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(&sm750_dev->accel,
>+   base, pitch, Bpp,
>+   region->dx, region->dy,
>+   region->width, region->height,
>+   color, rop);
>+
>   spin_unlock(&sm750_dev->slock);
>+  } else {
>+  sm750_dev->accel.de_fillrect(&sm750_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(&sm750_dev->slock);
> 
>-  sm750_dev->accel.de_copyarea(&sm750_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(&sm750_dev->accel,
>+   base, pitch, region->sx, 
>region->sy,
>+   base, pitch, Bpp, region->dx, 
>region->dy,
>+   region->width, region->height,
>+   HW_ROP2_COPY);
>+
>   spin_unlock(&sm750_dev->slock);
>+  } else {
>+  sm750_dev->accel.de_copyarea(&sm750_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(&sm750_dev->slock);
> 
>-  sm750_dev->accel.de_imageblit(&sm750_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(&sm750_dev->accel,
>+image->data, image->width >> 3, 0,
>+base, pitch, Bpp,
>+  

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(&usbin->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  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 
---
 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(&buf[pos], &msg->header, sizeof(msg->header));
+   pos += sizeof(msg->header);
 
len -= 2;
memcpy(&buf[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,   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(&wordbuf[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  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] 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 
---
 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  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  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  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
 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

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

[Frans Klaver]

On Wed, Aug 17, 2016 at 12:34 PM, Heikki Krogerus
 wrote:
> The purpose of USB Type-C connector class is to provide
> unified interface for the user space to get the status and
> basic information about USB Type-C connectors on a system,
> control over data role swapping, and when the port supports
> USB Power Delivery, also control over power role swapping
> and Alternate Modes.
>
> Signed-off-by: Heikki Krogerus 
> ---
>  Documentation/ABI/testing/sysfs-class-typec |  199 +
>  Documentation/usb/typec.txt |  103 +++
>  MAINTAINERS |9 +
>  drivers/usb/Kconfig |2 +
>  drivers/usb/Makefile|2 +
>  drivers/usb/typec/Kconfig   |7 +
>  drivers/usb/typec/Makefile  |1 +
>  drivers/usb/typec/typec.c   | 1104 
> +++
>  include/linux/usb/typec.h   |  260 +++
>  9 files changed, 1687 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-typec
>  create mode 100644 Documentation/usb/typec.txt
>  create mode 100644 drivers/usb/typec/Kconfig
>  create mode 100644 drivers/usb/typec/Makefile
>  create mode 100644 drivers/usb/typec/typec.c
>  create mode 100644 include/linux/usb/typec.h
>
> diff --git a/Documentation/ABI/testing/sysfs-class-typec 
> b/Documentation/ABI/testing/sysfs-class-typec
> new file mode 100644
> index 000..e6179d3
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-typec
> @@ -0,0 +1,199 @@
> +USB Type-C port devices (eg. /sys/class/typec/usbc0/)
> +
> +What:  /sys/class/typec//current_data_role
> +Date:  June 2016
> +Contact:   Heikki Krogerus 
> +Description:
> +   The current USB data role the port is operating in. This
> +   attribute can be used for requesting data role swapping on the
> +   port.
> +
> +   Valid values:
> +   - host
> +   - device
> +
> +What:  /sys/class/typec//current_power_role
> +Date:  June 2016
> +Contact:   Heikki Krogerus 
> +Description:
> +   The current power role of the port. This attribute can be used
> +   to request power role swap on the port when the port supports
> +   USB Power Delivery.
> +
> +   Valid values:
> +   - source
> +   - sink
> +
> +What:  /sys/class/typec//current_vconn_role
> +Date:  June 2016
> +Contact:   Heikki Krogerus 
> +Description:
> +   Shows the current VCONN role of the port. This attribute can 
> be
> +   used to request VCONN role swap on the port when the port
> +   supports USB Power Delivery.
> +
> +   Valid values are:
> +   - source
> +   - sink
> +
> +What:  /sys/class/typec//power_operation_mode
> +Date:  June 2016
> +Contact:   Heikki Krogerus 
> +Description:
> +   Shows the current power operational mode the port is in.
> +
> +   Valid values:
> +   - USB - Normal power levels defined in USB specifications
> +   - BC1.2 - Power levels defined in Battery Charging 
> Specification
> + v1.2
> +   - USB Type-C 1.5A - Higher 1.5A current defined in USB Type-C
> +   specification.
> +   - USB Type-C 3.0A - Higher 3A current defined in USB Type-C
> +   specification.
> +- USB Power Delivery - The voltages and currents defined in 
> USB
> +  Power Delivery specification
> +
> +What:  /sys/class/typec//preferred_role
> +Date:  June 2016
> +Contact:   Heikki Krogerus 
> +Description:
> +   The user space can notify the driver about the preferred role.
> +   It should be handled as enabling of Try.SRC or Try.SNK, as
> +   defined in USB Type-C specification, in the port drivers. By
> +   default there is no preferred role.
> +
> +   Valid values:
> +   - host
> +   - device
> +   - For example "none" to remove preference (anything else 
> except
> + "host" or "device")
> +
> +What:  /sys/class/typec//supported_accessory_modes
> +Date:  June 2016
> +Contact:   Heikki Krogerus 
> +Description:
> +   Lists the Accessory Modes, defined in the USB Type-C
> +   specification, the port supports.
> +
> +What:  /sys/class/typec//supported_data_roles
> +Date:  June 2016
> +Contact:   Heikki Krogerus 
> +Description:
> +   Lists the USB data roles the port is capable of supporting.
> +
> +   Valid values:
> +   - device
> +   - host
> +   - device, host

Re: [RFC PATCH] fbdev: add support for Sigma Designs' smp8xxxfb.ko

[Frans Klaver]

On Tue, Dec 29, 2015 at 3:06 PM, Sebastian Frias  wrote:
> On 12/29/2015 02:49 PM, Frans Klaver wrote:
>>
>> On Tue, Dec 29, 2015 at 2:15 PM, Sebastian Frias  wrote:
>>>
>>> Hi,
>>>
>>> We are wondering what is the recommended way of adding support for a
>>> framebuffer driver on the Linux kernel.
>>> Below you can find a patch with a proposed solution.
>>
>>
>> That's not really a solution to add a driver to the kernel. You'd have
>> to include some actual driver code as well.
>
>
> We are not attempting to upstream the driver yet, that's why its code is not
> included.
>
> The patch is an attempt to allow the user to enable Framebuffer support by
> providing an option for the user and then setting FB_CFB_FILLRECT,
> FB_CFB_COPYAREA and FB_CFB_IMAGEBLIT to yes.
>
> It does the job, but we feel (and you have sort of confirmed it) that it may
> not be a good idea to do it that way.
>
>>
>>
>>> Our frambuffer driver source code is provided separately, but right now
>>> it
>>> requires "cfb_fillrect", "cfb_copyarea" and "cfb_imageblit" to be
>>> provided
>>> by the kernel.
>>>
>>> Our current kernel fork (based on 3.4) hardcodes FB_CFB_FILLRECT,
>>> FB_CFB_COPYAREA and FB_CFB_IMAGEBLIT to yes.
>>> Since we are in the process of migrating to 4.x and upstreaming changes
>>> along the way, we would like to know if the patch below is the way to go
>>> with it or if you have suggestions to improve it.
>>
>>
>> Is the below patch really a patch you intend to upstream, or are you
>> just wondering about what your Kconfig entry should look like when you
>> upstream your driver?
>
>
> Right now we don't know if the driver will be upstreamed.
> Let me rephrase my question:
>
> - how would you recommend enabling FB_CFB_FILLRECT, FB_CFB_COPYAREA and
> FB_CFB_IMAGEBLIT for a driver that is not included in the kernel's tree?
>
> If that is not possible, I guess we will have to keep a forked tree until
> the driver is upstreamed, but we would like to avoid that, hence the
> question.

I guess you'll have to keep on doing that indeed. I'm not aware of any
location where out-of-tree drivers are considered. You should try
again when you have some actual code to upstream.

Cheers,
Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH] fbdev: add support for Sigma Designs' smp8xxxfb.ko

[Frans Klaver]

On Tue, Dec 29, 2015 at 2:15 PM, Sebastian Frias  wrote:
> Hi,
>
> We are wondering what is the recommended way of adding support for a
> framebuffer driver on the Linux kernel.
> Below you can find a patch with a proposed solution.

That's not really a solution to add a driver to the kernel. You'd have
to include some actual driver code as well.


> Our frambuffer driver source code is provided separately, but right now it
> requires "cfb_fillrect", "cfb_copyarea" and "cfb_imageblit" to be provided
> by the kernel.
>
> Our current kernel fork (based on 3.4) hardcodes FB_CFB_FILLRECT,
> FB_CFB_COPYAREA and FB_CFB_IMAGEBLIT to yes.
> Since we are in the process of migrating to 4.x and upstreaming changes
> along the way, we would like to know if the patch below is the way to go
> with it or if you have suggestions to improve it.

Is the below patch really a patch you intend to upstream, or are you
just wondering about what your Kconfig entry should look like when you
upstream your driver?


> Subject: [RFC PATCH] fbdev: add support for Sigma Designs' smp8xxxfb.ko
>
> Signed-off-by: Sebastian Frias 
> ---
>  drivers/video/fbdev/Kconfig |   10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
> index e6d16d6..46c4ab2 100644
> --- a/drivers/video/fbdev/Kconfig
> +++ b/drivers/video/fbdev/Kconfig
> @@ -615,6 +615,16 @@ config FB_BF537_LQ035
>   To compile this driver as a module, choose M here: the
>   module will be called bf537-lq035.
>
> +config FB_TANGO
> +   bool "Sigma Designs FrameBuffer support"
> +   depends on FB && ARCH_TANGO
> +   select FB_CFB_FILLRECT
> +   select FB_CFB_COPYAREA
> +   select FB_CFB_IMAGEBLIT
> +   help
> + You need to enable this if you intend to use Sigma
> + Designs' smp8xxxfb.ko driver.
> +
>  config FB_BFIN_7393
> tristate "Blackfin ADV7393 Video encoder"
> depends on FB && BLACKFIN

Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: mtd, nand, omap2: parse cmdline partition fail

[Frans Klaver]

On Thu, Dec 10, 2015 at 12:19 AM, Brian Norris
 wrote:
> On Fri, Dec 04, 2015 at 09:42:06AM +0100, Heiko Schocher wrote:
>> It seems to me add_mtd_device() gets only called for the mtd partitions
>> parsed from the cmdline ...
>
> That's true, when CONFIG_MTD_PARTITIONED_MASTER=n. (I'm really thinking
> we should accelerate the adoption of PARTITIONED_MASTER... maybe set it
> to default =y?)
>
> But even with CONFIG_MTD_PARTITIONED_MASTER=y we still have a problem.
>
> [...]
>
>> >The fact that this produces different names for you is slightly
>> >surprising to me, unless mtd->name is already set to something by the
>> >time it reaches add_mtd_device(). Or I overlooked something, which is
>> >entirely plausible as well.
>> >
>> >So effectively this should be the same as doing:
>> >
>> >   mtd->dev.parent = &pdev->dev;
>> >   mtd->name = dev_name(mtd->dev.parent);
>
> Yes, except for one thing (and this is the key): the mtd->name only gets
> set *after* the partitions are parsed, using the method from commit
> 807f16d4db95 ("mtd: core: set some defaults when dev.parent is set"). So
> the parsers (including cmdlinepart) get run with a blank (NULL) name,
> and you can't detect any partitions, since the name match will never
> work.

Right, that was something we overlooked earlier.


> I have a hack of a patch below (untested) that would hopefully work
> (based on current l2-mtd.git). I could port this to a base on 4.4-rc1 if
> it works OK, so we can get the regression fixed in this cycle.

That would be nice.


>> >>But wondering, if there are two or more identical nand chips in the
>> >>system, they will have the same mtd->name ... which seems buggy to me...
>> >
>> >Agree.
>>
>> Good, so we must fix it ;-)
>
> Yeah, that's a bad problem. Most people only plan for one chip and then
> realize they need 2 later. nand_base should probably support something
> to do this easily. Unfortunately, making a change like that could cause
> some problems with cmdline naming across kernel versions, if we start
> changing the convention for some drivers...(do we consider the MTD name
> part of the ABI?)

I would expect a name to be just a name; something parsable by humans.
By definition that cannot be an ABI. On the other hand, maybe it has
grown to become part of the ABI.


> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 89d811e7b04a..185dc36c687f 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -592,6 +592,15 @@ int mtd_device_parse_register(struct mtd_info *mtd, 
> const char * const *types,
> struct mtd_partitions parsed;
> int ret;
>
> +   if (mtd->dev.parent) {
> +   if (!mtd->owner && mtd->dev.parent->driver)
> +   mtd->owner = mtd->dev.parent->driver->owner;
> +   if (!mtd->name)
> +   mtd->name = dev_name(mtd->dev.parent);
> +   } else {
> +   pr_debug("mtd device won't show a device symlink in sysfs\n");
> +   }
> +
> memset(&parsed, 0, sizeof(parsed));
>
> ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);

This was the first thing I thought of when this issue was brought up.
If you do this, do you still need the chunk of code you copied from in
add_mtd_device()? Since we fill in these things on the master, I
wouldn't think we do.

Thanks,
Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: mtd, nand, omap2: parse cmdline partition fail

[Frans Klaver]

On Fri, Dec 4, 2015 at 7:48 AM, Heiko Schocher  wrote:
> Hello Frans,
>
> I just tried current mainline kernel:
> commit 2255702db4014d1c69d6037ed7bdad2d2e271985
> Merge: 9e5d25e c86576e
> Author: Linus Torvalds 
> Date:   Mon Nov 30 16:06:44 2015 -0800
>
> Merge tag 'mn10300-for-linus-v4.4-rc4' of
> git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging
>
> on an am3517 based board (mainlining soon). And with your commit:
> commit 853f1c58c4b2: mtd: nand: omap2: show parent device structure in sysfs
>
> MTD partitions from cmdline are not longer detected:
>
> [2.087305] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xcc
> [2.094097] nand: Micron MT29F4G16ABADAWP
> [2.098303] nand: 512 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB
> size: 64
> [2.106296] nand: WARNING: MT29F4G16ABADAWP: the ECC used on your system
> is too weak compared to the one required by the NAND chip
> [2.118674] MT29F4G16ABADAWP: 'partitions' subnode not found on
> /ocp/gpmc@6e00/nand@0,0. Trying to parse direct subnodes as partitions.
> [...]
>
> before this patch it worked:
> [2.307444] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xcc
> [2.314092] nand: Micron MT29F4G16ABADAWP
> [2.318348] nand: 512 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB
> size: 64
> [2.326331] nand: WARNING: omap2-nand.0: the ECC used on your system is
> too weak compared to the one required by the NAND chip
> [2.338336] 5 cmdlinepart partitions found on MTD device omap2-nand.0
> [2.345129] Creating 5 MTD partitions on "omap2-nand.0":
> [2.350704] 0x-0x0008 : "MLO"
> [2.366877] 0x0008-0x0018 : "u-boot"
> [2.379179] 0x0018-0x001c : "env1"
> [2.390627] 0x001c-0x0020 : "env2"
> [2.402255] 0x0020-0x2000 : "common_data"
>
> Reason is taht the mtd->name has changed from "omap2-nand.0" to the
> nand chip name ...
>
> If I revert this part from the patch
>
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 93f664c..28dcf66 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -1685,6 +1685,7 @@ static int omap_nand_probe(struct platform_device
> *pdev)
> info->ecc_opt   = pdata->ecc_opt;
> mtd = &info->mtd;
> mtd->priv   = &info->nand;
> +   mtd->name   = dev_name(&pdev->dev);
> mtd->dev.parent = &pdev->dev;
> nand_chip   = &info->nand;
> nand_chip->ecc.priv = NULL;
>
> It works again ...
>
> So the question is, is it intended to change the "mtd->name"?

That's definitely not intended. The expectation with this patch is
that nothing really changes, except that a parent device link is
available in sysfs. For the name this patch depends on 807f16d4db956
("mtd: core: set some defaults when dev.parent is set") which does
something like:

if (mtd->dev.parent) {
if (!mtd->name)
mtd->name = dev_name(mtd->dev.parent);
}

The fact that this produces different names for you is slightly
surprising to me, unless mtd->name is already set to something by the
time it reaches add_mtd_device(). Or I overlooked something, which is
entirely plausible as well.

So effectively this should be the same as doing:

  mtd->dev.parent = &pdev->dev;
  mtd->name = dev_name(mtd->dev.parent);


> But wondering, if there are two or more identical nand chips in the
> system, they will have the same mtd->name ... which seems buggy to me...

Agree.

Thanks,
Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mtd: nand: mxc_nand: fix a possible NULL dereference

[Frans Klaver]

On Thu, Nov 12, 2015 at 9:53 AM, Uwe Kleine-König
 wrote:
> CC += devicet...@vger.kernel.org, gregkh

You added linux@pengutronix instead of devicetree.

>
> On Thu, Nov 12, 2015 at 09:36:55AM +0100, Frans Klaver wrote:
>> On Thu, Nov 12, 2015 at 9:26 AM, Uwe Kleine-König
>>  wrote:
>> > On Thu, Nov 12, 2015 at 09:03:11AM +0100, Frans Klaver wrote:
>> >> Hi,
>> >>
>> >> On Thu, Nov 12, 2015 at 8:46 AM, LABBE Corentin
>> >>  wrote:
>> >> > of_match_device could return NULL, and so cause a NULL pointer
>> >> > dereference later.
>> >>
>> >> Did you actually run into this? It seems to me that this driver is
>> >> only probed if and only if we have a match and that therefore
>> >> of_match_device will always return a valid pointer (it is using the
>> >> same match table). Am I missing something?
>> >
>> > Yes, you're missing something. The driver would probe for a dt snippet
>> > like:
>> >
>> > mxc_nand {
>> > compatible = "foobar";
>> > }
>> >
>> > In this case dev->of_node is non-NULL but of_match_device(mxcnd_dt_ids,
>> > dev) is.
>> >
>> > (I didn't actually test this, so there is a chance I'm wrong here. And
>> > if not I wonder if it is sensible at all to match the device name on
>> > driver name for of-created platform devices.)
>>
>> Yea, looks like you're right. platform devices check a number of
>> things to determine a match, among which is driver name if all else
>> fails (platform.c, platform_match()).
>
> Maybe something like this would help to reduce surprises:
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index f80aaaf9f610..a9fc22c86552 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -840,8 +840,8 @@ static int platform_match(struct device *dev, struct 
> device_driver *drv)
> return !strcmp(pdev->driver_override, drv->name);
>
> /* Attempt an OF style match first */
> -   if (of_driver_match_device(dev, drv))
> -   return 1;
> +   if (pdev->dev.of_node)
> +   return of_driver_match_device(dev, drv);
>
> /* Then try ACPI style match */
> if (acpi_driver_match_device(dev, drv))

That looks sensible, yea. There is a chance that misbehaving DT nodes
will fail after this change, of course.

Thanks,
Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mtd: nand: mxc_nand: fix a possible NULL dereference

[Frans Klaver]

On Thu, Nov 12, 2015 at 9:26 AM, Uwe Kleine-König
 wrote:
> On Thu, Nov 12, 2015 at 09:03:11AM +0100, Frans Klaver wrote:
>> Hi,
>>
>> On Thu, Nov 12, 2015 at 8:46 AM, LABBE Corentin
>>  wrote:
>> > of_match_device could return NULL, and so cause a NULL pointer
>> > dereference later.
>>
>> Did you actually run into this? It seems to me that this driver is
>> only probed if and only if we have a match and that therefore
>> of_match_device will always return a valid pointer (it is using the
>> same match table). Am I missing something?
>
> Yes, you're missing something. The driver would probe for a dt snippet
> like:
>
> mxc_nand {
> compatible = "foobar";
> }
>
> In this case dev->of_node is non-NULL but of_match_device(mxcnd_dt_ids,
> dev) is.
>
> (I didn't actually test this, so there is a chance I'm wrong here. And
> if not I wonder if it is sensible at all to match the device name on
> driver name for of-created platform devices.)

Yea, looks like you're right. platform devices check a number of
things to determine a match, among which is driver name if all else
fails (platform.c, platform_match()).

Thanks,
Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mtd: nand: mxc_nand: fix a possible NULL dereference

[Frans Klaver]

Hi,

On Thu, Nov 12, 2015 at 8:46 AM, LABBE Corentin
 wrote:
> of_match_device could return NULL, and so cause a NULL pointer
> dereference later.

Did you actually run into this? It seems to me that this driver is
only probed if and only if we have a match and that therefore
of_match_device will always return a valid pointer (it is using the
same match table). Am I missing something?


> Signed-off-by: LABBE Corentin 
> ---
>  drivers/mtd/nand/mxc_nand.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index 136e73a..9e42431 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -1464,8 +1464,7 @@ static int __init mxcnd_probe_dt(struct mxc_nand_host 
> *host)
>  {
> struct device_node *np = host->dev->of_node;
> struct mxc_nand_platform_data *pdata = &host->pdata;
> -   const struct of_device_id *of_id =
> -   of_match_device(mxcnd_dt_ids, host->dev);
> +   const struct of_device_id *of_id;
> int buswidth;
>
> if (!np)
> @@ -1482,6 +1481,9 @@ static int __init mxcnd_probe_dt(struct mxc_nand_host 
> *host)
>
> pdata->width = buswidth / 8;
>
> +   of_id = of_match_device(mxcnd_dt_ids, host->dev);
> +   if (!of_id)
> +   return -ENODEV;
> host->devtype_data = of_id->data;
>
> return 0;
> --
> 2.4.10
>

Thanks,
Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 45/79] linux/if.h linux/hdlc/ioctl.h: move IFNAMSIZ definition to hdlc/ioctl.h

[Frans Klaver]

Hi,

On Thu, Oct 15, 2015 at 7:56 AM, Mikko Rapeli  wrote:
> And include linux/hdlc/ioctl.h from linux/if.h.

That appears to have already been the case before this patch. You just
add a comment behind the include statement.


> diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h
> index 9cf2394..0f98f0a 100644
> --- a/include/uapi/linux/if.h
> +++ b/include/uapi/linux/if.h
> @@ -22,10 +22,8 @@
>  #include/* for "__kernel_caddr_t" et al */
>  #include   /* for "struct sockaddr" et al  */
>  #include /* for "__user" et al   */
> -
> -#defineIFNAMSIZ16
> -#defineIFALIASZ256
> -#include 
> +#include/* for IFNAMSIZ */
> +#defineIFALIASZ256

Thanks,
Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 05/60] mtd: devices: m25p80: show parent device in sysfs

[Frans Klaver]

On Tue, Oct 13, 2015 at 7:37 PM, Brian Norris
 wrote:
> On Wed, Jun 10, 2015 at 10:38:19PM +0200, Frans Klaver wrote:
>> Fix a bug where mtd parent device symlinks aren't shown in sysfs.
>>
>> Signed-off-by: Frans Klaver 
>
> This is one of the patches that didn't apply...
>
>> ---
>>  drivers/mtd/devices/m25p80.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>> index 3af137f..dfa6ee0 100644
>> --- a/drivers/mtd/devices/m25p80.c
>> +++ b/drivers/mtd/devices/m25p80.c
>> @@ -206,6 +206,7 @@ static int m25p_probe(struct spi_device *spi)
>>
>>   spi_set_drvdata(spi, flash);
>>   flash->mtd.priv = nor;
>> + flash->mtd.dev.parent = &spi->dev;
>
> ...but I don't think this patch is actually necessary. See
> spi_nor_scan(), which assigns mtd->dev.parent for us. So as long as the
> driver assigns the spi_nor::dev field, the spi-nor framework handles
> this for us.
>
> Same comment applies to fsl-quadspi.c (patch 59), which also didn't
> apply cleanly. So I don't think we need either of these patches.

Sure. Feel free to drop them. The main reason these patches exist is
based on the fact that these files didn't contain the "dev.parent ="
pattern. I tried to only add those necessary based on code inspection,
so it is very well possible that we don't need them. If someone runs
into it on these drivers, that's the time to fix it I guess.

Thanks,
Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] arm: omap2: vc: fix 'or' always true warning

[Frans Klaver]

On Tue, Sep 1, 2015 at 5:46 PM, Tony Lindgren  wrote:
> * Frans Klaver  [150828 03:14]:
>> From: Frans Klaver 
>>
>> Fix the warning:
>> arch/arm/mach-omap2/vc.c:302:47: warning: logical ‘or’ of collectively 
>> exhaustive tests is always true [-Wlogical-op]
>>
>> As we're toggling both CLKREQ and OFFMODE, we should also be checking
>> OFFMODE.
>>
>> Signed-off-by: Frans Klaver 
>
> Thanks apppying into omap-for-v4.3/fixes.

Thanks!

Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] arm: omap2: vc: fix 'or' always true warning

[Frans Klaver]

From: Frans Klaver 

Fix the warning:
arch/arm/mach-omap2/vc.c:302:47: warning: logical ‘or’ of collectively 
exhaustive tests is always true [-Wlogical-op]

As we're toggling both CLKREQ and OFFMODE, we should also be checking
OFFMODE.

Signed-off-by: Frans Klaver 
---
 arch/arm/mach-omap2/vc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-omap2/vc.c b/arch/arm/mach-omap2/vc.c
index 076fd20d7e5a..807bc79e3e3d 100644
--- a/arch/arm/mach-omap2/vc.c
+++ b/arch/arm/mach-omap2/vc.c
@@ -300,7 +300,7 @@ static void __init omap3_vc_init_pmic_signaling(struct 
voltagedomain *voltdm)
 
val = voltdm->read(OMAP3_PRM_POLCTRL_OFFSET);
if (!(val & OMAP3430_PRM_POLCTRL_CLKREQ_POL) ||
-   (val & OMAP3430_PRM_POLCTRL_CLKREQ_POL)) {
+   (val & OMAP3430_PRM_POLCTRL_OFFMODE_POL)) {
val |= OMAP3430_PRM_POLCTRL_CLKREQ_POL;
val &= ~OMAP3430_PRM_POLCTRL_OFFMODE_POL;
pr_debug("PM: fixing sys_clkreq and sys_off_mode polarity to 
0x%x\n",
-- 
2.3.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] fs/kcore: change copy_to_user to copy_in_user

[Frans Klaver]

On Thu, Aug 20, 2015 at 10:59 AM, yalin wang  wrote:
> the copy_to_user() here expect can fix the fault on both kernel and
> user address, this is not true on other platforms except x86,
> change to user copy_in_user() so that can detect the page fault,
> work as expected.

Could you rephrase this into multiple sentences in comprehensible
English? What is the expected behavior, what is the unexpected
behavior and what can people do to trigger it?


> Signed-off-by: yalin wang 
> ---
>  fs/proc/kcore.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
> index 92e6726..4f28deb 100644
> --- a/fs/proc/kcore.c
> +++ b/fs/proc/kcore.c
> @@ -515,8 +515,12 @@ read_kcore(struct file *file, char __user *buffer, 
> size_t buflen, loff_t *fpos)
> } else {
> if (kern_addr_valid(start)) {
> unsigned long n;
> -
> -   n = copy_to_user(buffer, (char *)start, tsz);
> +   if ((start + tsz < tsz) ||
> +   (start + tsz) > TASK_SIZE)
> +   return -EFAULT;
> +   set_fs(KERNEL_DS);
> +   n = copy_in_user(buffer, (char *)start, tsz);
> +   set_fs(USER_DS);
> /*
>  * We cannot distinguish between fault on 
> source
>  * and fault on destination. When this happens
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/9] staging: most: NULL comparison style

[Frans Klaver]

On Wed, Aug 19, 2015 at 12:00 PM, Sudip Mukherjee
 wrote:
> On Tue, Aug 18, 2015 at 01:31:00PM -0300, Fabio Estevam wrote:
>> On Tue, Aug 18, 2015 at 12:18 PM, Sudip Mukherjee
>>  wrote:
>> > According to the kernel coding style the NULL check should not be
>> > written as [variable] == NULL or [variable] != NULL.
>>
>> It seems this not documented in Documentation/CodingStyle .
> Yes, it is not in the CodingStyle file. But mostly it is the convention
> that is followed. And in CodingStyle file if you see the "The rationale
> for using gotos is:" section, you will see in the example function the
> test is done like: if (!buffer).
> Anyways, frankly speaking I know commit message is bad but I could not
> think of anything else other than the one I wrote. Any ideas please...

Maybe something like:

Although not made explicit in Documentation/CodingStyle, there seems
to be a preference for writing NULL tests as 'if (!var)' rather than
if (var == NULL). The coding style does explicitly prefer brevity, so
convert all NULL checks to follow this shorter approach.

Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv6 2/5] Staging: most: mostcore/core.c. Fix "Using plain integer as NULL pointer" warnings

[Frans Klaver]

On Fri, Aug 14, 2015 at 11:42 AM, Adrian Remonda
 wrote:
> This patch fixes the warning generated by sparse: "Using plain integer
> as NULL pointer" by replacing the offending 0 with NULL.
>
> Signed-off-by: Adrian Remonda 
> ---
>  v6: styling. Change the NULL test to comply with the Kernel coding style

Your commit message no longer fits the actual change...


>  drivers/staging/most/mostcore/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/most/mostcore/core.c 
> b/drivers/staging/most/mostcore/core.c
> index fa9c19b65a5c..7bb16db42893 100644
> --- a/drivers/staging/most/mostcore/core.c
> +++ b/drivers/staging/most/mostcore/core.c
> @@ -982,7 +982,7 @@ static ssize_t store_add_link(struct most_aim_obj 
> *aim_obj,
> if (ret)
> return ret;
>
> -   if (mdev_devnod == 0 || *mdev_devnod == 0) {
> +   if (!mdev_devnod || *mdev_devnod == 0) {
> snprintf(devnod_buf, sizeof(devnod_buf), "%s-%s", mdev, 
> mdev_ch);
> mdev_devnod = devnod_buf;
> }
> --

Thanks,
Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Staging: media/bcm2048: Fix line over 80 characters warning as detected by checkpatch.pl

[Frans Klaver]

On Wed, Aug 12, 2015 at 1:19 PM, Pali Rohár  wrote:
> On Wednesday 12 August 2015 11:12:49 Shah, Yash (Y.) wrote:
>> From: Yash Shah 
>>
>> Fix line over 80 characters warning as detected by checkpatch.pl
>>
>> Signed-off-by: Yash Shah 
>> ---
>>  drivers/staging/media/bcm2048/radio-bcm2048.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/media/bcm2048/radio-bcm2048.c 
>> b/drivers/staging/media/bcm2048/radio-bcm2048.c
>> index 8bc68e2..d36350e 100644
>> --- a/drivers/staging/media/bcm2048/radio-bcm2048.c
>> +++ b/drivers/staging/media/bcm2048/radio-bcm2048.c
>> @@ -2243,7 +2243,8 @@ static ssize_t bcm2048_fops_read(struct file *file, 
>> char __user *buf,
>>
>>   tmpbuf[i] = bdev->rds_info.radio_text[bdev->rd_index+i+2];
>>   tmpbuf[i+1] = bdev->rds_info.radio_text[bdev->rd_index+i+1];
>> - tmpbuf[i+2] = (bdev->rds_info.radio_text[bdev->rd_index + i] & 
>> 0xf0) >> 4;
>> + tmpbuf[i+2] = (bdev->rds_info.radio_text[bdev->rd_index + i]
>> +  & 0xf0) >> 4;
>>   if ((bdev->rds_info.radio_text[bdev->rd_index+i] &
>>   BCM2048_RDS_CRC_MASK) == BCM2048_RDS_CRC_UNRECOVARABLE)
>>   tmpbuf[i+2] |= 0x80;
>
> Hi! I think that code after this change is less readable as before.

I agree. I would do something about 'bdev->rds_info.radio_text'
instead and shorten all three lines.

Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [Telephony/MUX]: solve checkpatch issues

[Frans Klaver]

On Mon, Aug 10, 2015 at 2:50 PM, Gwenn Bourree  wrote:
> In order to prepare the submission of other functional
> patches, the checkpatch script has been applied and the
> reported issues have been solved.

Which warnings? Explain why these changes are an improvement. Just the
fact that checkpatch complains is sometimes not enough to warrant the
change.

I would expect this to be done in a series, rather than in one patch.


> Signed-off-by: Gwenn Bourree 
> ---
>  drivers/tty/n_gsm.c | 56 
> +
>  1 file changed, 44 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 382d3fc..2983ae2 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -486,10 +486,11 @@ static void gsm_print_packet(const char *hdr, int addr, 
> int cr,
> pr_cont("UIH");
> break;
> default:
> -   if (!(control & 0x01)) {
> +   if (!(control & 0x01))
> pr_cont("I N(S)%d N(R)%d",
> (control & 0x0E) >> 1, (control & 0xE0) >> 5);
> -   } else switch (control & 0x0F) {
> +   else
> +   switch (control & 0x0F) {
> case RR:
> pr_cont("RR(%d)", (control & 0xE0) >> 5);
> break;
> @@ -501,7 +502,7 @@ static void gsm_print_packet(const char *hdr, int addr, 
> int cr,
> break;
> default:
> pr_cont("[%02X]", control);
> -   }
> +   }
> }

Does this improve readability?


> @@ -783,6 +786,7 @@ static void __gsm_data_queue(struct gsm_dlci *dlci, 
> struct gsm_msg *msg)
>  static void gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg)
>  {
> unsigned long flags;
> +
> spin_lock_irqsave(&dlci->gsm->tx_lock, flags);
> __gsm_data_queue(dlci, msg);
> spin_unlock_irqrestore(&dlci->gsm->tx_lock, flags);
> @@ -833,7 +837,8 @@ static int gsm_dlci_data_output(struct gsm_mux *gsm, 
> struct gsm_dlci *dlci)
> *dp++ = gsm_encode_modem(dlci);
> break;
> }
> -   WARN_ON(kfifo_out_locked(dlci->fifo, dp , len, &dlci->lock) 
> != len);
> +   WARN_ON(kfifo_out_locked(dlci->fifo, dp, len,
> +   &dlci->lock) != len);

The second line should be
&dlci->lock

Again I wonder if readability is actually improving by this change.
Maybe things improve if the line break is done somewhere else?

Thanks,
Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] power_supply: Adjust devm usage

[Frans Klaver]

On Fri, Jul 24, 2015 at 2:26 PM, Sebastian Reichel  wrote:
> Hi,
>
> Thanks for the cleanup patch.
> I have a couple of comments inlined.
>
>> Subject: Re: [PATCH] power_supply: Adjust devm usage
>
> Please make this "power_supply: bq24735: ...".
>
> On Fri, Jul 24, 2015 at 05:28:13PM +0530, Vaishali Thakkar wrote:
>> Use devm_kasprintf instead of kasprintf. Also, remove various
>> gotos by direct returns and drop unneeded label err_free_name.
>
> Please also use devm_power_supply_unregister() instead
> of power_supply_unregister() to further simplify the driver.

Sounds like a separate patch.

Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] power_supply: Adjust devm usage

[Frans Klaver]

Hi,

On Fri, Jul 24, 2015 at 1:58 PM, Vaishali Thakkar
 wrote:
> Use devm_kasprintf instead of kasprintf. Also, remove various
> gotos by direct returns and drop unneeded label err_free_name.

If there's to be a respin, reword this so that it becomes clearer that
removing the various gotos and the label is an effect of using
devm_kasprintf here. I started out thinking that this patch did two
things.

Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Fix one source file coding sytle issue.

[Frans Klaver]

Hi,

On Thu, Jul 23, 2015 at 8:20 AM, Incarnation P. Lee
 wrote:
> drivers/staging/lustre/lustre/obdclass/cl_page.c

It is custom that you write slightly more introductory text here. Just
a file name is pointless.

The subject should probably contain [PATCH 0/5]. This would be
automatically fixed for you when using 'git format-patch
--cover-letter ...'.

The other patches should at least mention the area of the kernel the
change is done in:

  [PATCH N/5] staging: lustre: fix a missing space after comma

So that it becomes immediately clear where the change is done, and
what it is supposed to achieve.


> Signed-off-by: Incarnation P. Lee 

That's not necessary in the cover letter.

Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/4] serial: 8250_dw: Add UPF_SKIP_TEST to flags depend on device tree

[Frans Klaver]

On Thu, Jul 23, 2015 at 8:21 AM, Noam Camus  wrote:
> From: Peter Hurley [mailto:pe...@hurleysoftware.com]
> Sent: Wednesday, July 22, 2015 3:21 PM
>
>>>On 07/22/2015 05:34 AM, Noam Camus wrote:
>>> From: Noam Camus 
>>>
>>> Add support for OF option "no-loopback-test"
>
>> Changes to devicetree need to at least get acks from DT maintainers.
> I will add them in my v2
> Should I take this patch apart from this set or should I add DT maintainers 
> to whole set?

Add them at least to your cover letter and this one specifically. It's
a small set, so it's fine to add them to all patches, so they can see
the bigger picture. No need to take this patch out of the set (and
context).

Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 1/2] staging: rtl8192u: remove bool comparisons

[Frans Klaver]

On 20 July 2015 21:42:39 CEST, Luis de Bethencourt  
wrote:
>On Mon, Jul 20, 2015 at 09:54:56PM +0300, Dan Carpenter wrote:
>> On Mon, Jul 20, 2015 at 06:35:42PM +0200, Luis de Bethencourt wrote:
>> > Remove explicit true/false comparisons to bool variables.
>> > 
>> > Signed-off-by: Luis de Bethencourt 
>> > ---
>> 
>> Put a note here under the cut off why you are redoing this patch.
>> 
>> regards,
>> dan carpenter
>> 
>
>Hello Dan,
>
>I should've explained it after the cut off. Sorry.
>
>I redid patch 2/2 after a comment from Joe Perches. Then resubmitted
>both 1/2
>and 2/2 to keep them in sync. Not sure that is the correct way to do
>it.

Just mention that there are no changes since last time in that case. 

Frans 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Fix one file coding style issus of linux-next-20150708

[Frans Klaver]

Hi,

On Thu, Jul 16, 2015 at 8:17 AM, Incarnation P. Lee
 wrote:
> Signed-off-by: Li Pan (Incarnation P. Lee) 
>
> Fix one file coding sytle issue on linux-next-20150708, including macro
> aligned, missing blank lines after variable declarnation and brace missing
> in one line if structure.

Oh dear, what happened to only doing one thing per patch? And spell check?


> This patch can be one part of Eudyotula Task10.

How is that relevant?

Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [SCSI] FlashPoint: optimize string comparison

[Frans Klaver]

On Wed, Jul 8, 2015 at 7:45 AM, Christophe JAILLET
 wrote:
> Le 07/07/2015 19:04, Khalid Aziz a écrit :
>>
>> On 07/07/2015 02:45 AM, Frans Klaver wrote:
>>>
>>> On Tue, Jul 7, 2015 at 7:39 AM, Christophe JAILLET
>>>  wrote:
>>>>
>>>> Stop comparing the strings as soon as we know that they don't match.
>>>>
>>>> Signed-off-by: Christophe JAILLET 
>>>> ---
>>>>   drivers/scsi/FlashPoint.c | 4 +++-
>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/scsi/FlashPoint.c b/drivers/scsi/FlashPoint.c
>>>> index 5c74e4c..24a4d1a 100644
>>>> --- a/drivers/scsi/FlashPoint.c
>>>> +++ b/drivers/scsi/FlashPoint.c
>>>> @@ -6280,8 +6280,10 @@ static unsigned char FPT_scmachid(unsigned char
>>>> p_card,
>>>>  match = 1;
>>>>
>>>>  for (k = 0; k < ID_STRING_LENGTH; k++) {
>>>> -   if (p_id_string[k] !=
>>>> FPT_scamInfo[i].id_string[k])
>>>> +   if (p_id_string[k] !=
>>>> FPT_scamInfo[i].id_string[k]) {
>>>>  match = 0;
>>>> +   break;
>>>> +   }
>>>>  }
>>>>
>>>>  if (match) {
>>>
>>>
>>> Why doesn't this use strncmp?
>>>
>>> Thanks,
>>> Frans
>>>
>>
>> I suspect that is how this code came from Mylex many years ago. Using
>> strncmp would indeed be a better way to clean this up. Also, further down in
>> the same routine:
>>
>> if (FPT_scamInfo[match].state == ID_UNUSED) {
>> for (k = 0; k < ID_STRING_LENGTH; k++) {
>> FPT_scamInfo[match].id_string[k] =
>> p_id_string[k];
>> }
>>
>>
>> This should use strncpy instead. There is another similar spot further
>> down.
>>
>> Christophe, if you can send a new patch with these clean-ups, that would
>> be great.
>>
>> Thanks,
>> Khalid
>
>
> Hi,
>
> I'm sorry but I won't propose a new patch for that.
>
> I had the same reaction at first (why not use strncmp?) but it seems to be
> the way this driver is coded. Should we want to introduce strncmp here,
> then, as you have noticed, strcpy should be used to. memset could be also
> used in many places. Then looking elsewhere in the code, many things should,
> IMHO, also be fixed. (use consistently empty lines before/after code ; use
> consistently { }...)
>
> Another concern to me is "the use of carriage return". In the following
> examples, things could be much more readable if not limited to 50 chars per
> line, or so.

80. Parts of the code are indented way too much, resulting in these
unreadable lines. I have to say that this entire driver looks like
something that (sh|w)ould be in staging right now.

Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Clarification for the use of additional fields in the message body

[Frans Klaver]

On Tue, Jul 7, 2015 at 1:53 PM, SF Markus Elfring
 wrote:
>> I think that as far as these kernel mailing lists are concerned,
>> the date of the update suggestion is the date on which you submitted the 
>> patch,
>> rather than the date you originally committed it to your local tree.
>
> I imagine that there are committers who would like to keep
> corresponding software development history a bit more accurate.

I guess it depends on what your view on accurate is.


>> If you wish to keep track of this evolution for yourself, or
>> wish to share it, you're better off stashing it somewhere in a
>> (public) git repo that you control.
>
> Would it be nicer to preserve such data directly also
> by the usual mail interface?
>
>
>> If you insist on placing the date somewhere, you can also put the date
>> there if you wish. It'll be ignored by git when applied.
>
> This content management tool provides the capability to store
> the discussed information by the parameters "--author=" and "--date=",
> doesn't it?
> Is the environment variable "GIT_AUTHOR_DATE" also interesting occasionally?
>
> How often do you take extra care for passing appropriate data there?

I can't remember ever changing or explicitly preserving the commit
date. I don't think I care enough. I did change the author on botched
patches, but that's an exception. Remembering the author separately
from the committer is something git does by design anyway.

Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] megaraid:Remove no longer required variable ret from the function megasas_sync_map_info

[Frans Klaver]

Can't seem to find the original, so here's a reply to the ack mail.

On Tue, Jul 7, 2015 at 10:49 AM, Sumit Saxena
 wrote:
> -Original Message-
> From: Nicholas Krause [mailto:xerofo...@gmail.com]
> Sent: Monday, July 06, 2015 9:43 PM
> To: kashyap.de...@avagotech.com
> Cc: sumit.sax...@avagotech.com; uday.ling...@avagotech.com;
> jbottom...@odin.com; megaraidlinux@avagotech.com;
> linux-s...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH] megaraid:Remove no longer required variable ret from the
> function megasas_sync_map_info

Isn't something shorter like

  [PATCH] megaraid: remove pointless variable

much more readable?


> This removes the no longer required variable ret due to this variable only
> ever being used at the end of the function megasas_sync_map_info without
> changing it's value from the orginal setting of its value to zero due to
> this just remove the variable ret and just return the value of zero
> directly here in order to indicate to the caller the call to this function
> has run successfully without any non recoverable issues.

No interpunction?


> Signed-off-by: Nicholas Krause 
> ---
>  drivers/scsi/megaraid/megaraid_sas_fusion.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> index 46a0f8f..b5a8c65 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> @@ -836,7 +836,7 @@ megasas_get_map_info(struct megasas_instance
> *instance)  int  megasas_sync_map_info(struct megasas_instance *instance)
> {
> -   int ret = 0, i;
> +   int i;
> struct megasas_cmd *cmd;
> struct megasas_dcmd_frame *dcmd;
> u32 size_sync_info, num_lds;
> @@ -906,7 +906,7 @@ megasas_sync_map_info(struct megasas_instance
> *instance)
>
> instance->instancet->issue_dcmd(instance, cmd);
>
> -   return ret;
> +   return 0;
>  }
>
> Acked-by: Sumit Saxena 

This ack in an outlook-style response confused the hell out of me ;).

Thanks,
Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [SCSI] FlashPoint: optimize string comparison

[Frans Klaver]

On Tue, Jul 7, 2015 at 7:39 AM, Christophe JAILLET
 wrote:
> Stop comparing the strings as soon as we know that they don't match.
>
> Signed-off-by: Christophe JAILLET 
> ---
>  drivers/scsi/FlashPoint.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/FlashPoint.c b/drivers/scsi/FlashPoint.c
> index 5c74e4c..24a4d1a 100644
> --- a/drivers/scsi/FlashPoint.c
> +++ b/drivers/scsi/FlashPoint.c
> @@ -6280,8 +6280,10 @@ static unsigned char FPT_scmachid(unsigned char p_card,
> match = 1;
>
> for (k = 0; k < ID_STRING_LENGTH; k++) {
> -   if (p_id_string[k] != FPT_scamInfo[i].id_string[k])
> +   if (p_id_string[k] != FPT_scamInfo[i].id_string[k]) {
> match = 0;
> +   break;
> +   }
> }
>
> if (match) {

Why doesn't this use strncmp?

Thanks,
Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Clarification for the use of additional fields in the message body

[Frans Klaver]

On Tue, Jul 7, 2015 at 9:54 AM, SF Markus Elfring
 wrote:

>> No need to try and preserve it.
>
> I find that it might occasionally help to share and keep the record
> on timestamps about the evolution for an original update suggestion.

I think that as far as these kernel mailing lists are concerned, the
date of the update suggestion is the date on which you submitted the
patch, rather than the date you originally committed it to your local
tree. If you wish to keep track of this evolution for yourself, or
wish to share it, you're better off stashing it somewhere in a
(public) git repo that you control. If you wish, you can always make
mention of that repo below the ---, just above the diffstat. If you
insist on placing the date somewhere, you can also put the date there
if you wish. It'll be ignored by git when applied.

Cheers,
Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Clarification for the use of additional fields in the message body

[Frans Klaver]

On Tue, Jul 7, 2015 at 8:21 AM, SF Markus Elfring
 wrote:
>>> From: Markus Elfring 
>>> Date: Sat, 27 Jun 2015 15:56:57 +0200
>>
>> Why is this in the body of the email?
>
> Does the canonical patch format support to preserve
> specific details about a shown commit by specification
> of fields like "Date" and "From" in the message body?

Using "From:" in the body can be used to provide your properly spelled
name, or the name/e-mail of the actual author, if that happens not to
be the sender. git-send-email will do that automagically for you. If
"From:" is not specified in the message body, the e-mails sender will
be used as author.

The date, as far as I know, is ignored. It is the commit date, not the
authoring date, and once your patch is applied by a maintainer (i.e.
committed), the date gets reset anyway. No need to try and preserve
it.

Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 3/3] Staging: android: uapi: Fix coding style typedef issue

[Frans Klaver]

Subject suggestion: "staging: android: uapi: remove unnecessary typedef"

That makes it immediately clear what the issue is.


On Wed, Jul 1, 2015 at 3:47 PM, Sohny Thomas  wrote:
>
> As per checkpatch.pl remove unnecessary typedef
> Here ion_user_handle_t was used only in the header .
>
> Signed-of-by: Sohny Thomas 

Typo here as well.


> ---
>  drivers/staging/android/uapi/ion.h | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/android/uapi/ion.h 
> b/drivers/staging/android/uapi/ion.h
> index 68a14b4..9897403 100644
> --- a/drivers/staging/android/uapi/ion.h
> +++ b/drivers/staging/android/uapi/ion.h
> @@ -20,7 +20,6 @@
>  #include 
>  #include 
>
> -typedef int ion_user_handle_t;
>

I would expect an extra line to be removed here. This will leave two
empty lines.

>  /**
>   * enum ion_heap_types - list of all possible types of heaps
> @@ -88,7 +87,7 @@ struct ion_allocation_data {
> size_t align;
> unsigned int heap_id_mask;
> unsigned int flags;
> -   ion_user_handle_t handle;
> +   int handle;
>  };
>
>  /**
> @@ -102,7 +101,7 @@ struct ion_allocation_data {
>   * provides the file descriptor and the kernel returns the handle.
>   */
>  struct ion_fd_data {
> -   ion_user_handle_t handle;
> +   int handle;
> int fd;
>  };
>
> @@ -111,7 +110,7 @@ struct ion_fd_data {
>   * @handle:a handle
>   */
>  struct ion_handle_data {
> -   ion_user_handle_t handle;
> +   int handle;
>  };
>
>  /**
> --
>
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] Staging: android: ion: fix coding style

[Frans Klaver]

On Wed, Jul 1, 2015 at 3:46 PM, Sohny Thomas  wrote:
>
> - Fixed 80 char limit exceeding line
>   and a newline after declarations as per checkpatch.pl
>
> Signed-of-by: Sohny Thomas 

You can remove the dash, and fix the -off- typo (hint: use 'git commit
-s' for automating it). Maybe hint towards what coding style is fixed
in the summary:

staging: android: ion: fix line over 80 characters

> ---
>  drivers/staging/android/ion/ion.c| 1 +
>  drivers/staging/android/ion/ion_chunk_heap.c | 4 ++--
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion.c 
> b/drivers/staging/android/ion/ion.c
> index 6f48112..e44f5e6 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -1106,6 +1106,7 @@ struct dma_buf *ion_share_dma_buf(struct ion_client 
> *client,
> struct ion_buffer *buffer;
> struct dma_buf *dmabuf;
> bool valid_handle;
> +
> DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
>
> mutex_lock(&client->lock);
> diff --git a/drivers/staging/android/ion/ion_chunk_heap.c 
> b/drivers/staging/android/ion/ion_chunk_heap.c
> index 5474615..0813163 100644
> --- a/drivers/staging/android/ion/ion_chunk_heap.c
> +++ b/drivers/staging/android/ion/ion_chunk_heap.c
> @@ -173,8 +173,8 @@ struct ion_heap *ion_chunk_heap_create(struct 
> ion_platform_heap *heap_data)
> chunk_heap->heap.ops = &chunk_heap_ops;
> chunk_heap->heap.type = ION_HEAP_TYPE_CHUNK;
> chunk_heap->heap.flags = ION_HEAP_FLAG_DEFER_FREE;
> -   pr_debug("%s: base %lu size %zu align %ld\n", __func__, 
> chunk_heap->base,
> -   heap_data->size, heap_data->align);
> +   pr_debug("%s: base %lu size %zu align %ld\n", __func__,
> +   chunk_heap->base, heap_data->size, heap_data->align);
>
> return &chunk_heap->heap;
>
> --
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/3] Staging: Android: Fixes TODO file

[Frans Klaver]

On Wed, Jul 1, 2015 at 3:44 PM, Sohny Thomas  wrote:
>
> - removed non-existent issue from TODO file
> kuid_t or uid_t not present in staging/android
>
> Signed-of-by: Sohny Thomas 

s,-of-,-off-,

You can remove the leading dash (-). Could you elaborate on why this
issue is non existent?

Thanks,
Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Staging: android: fix coding style and TODO file

[Frans Klaver]

On Wed, Jul 1, 2015 at 2:22 PM, Sohny Thomas  wrote:
>
>
> On Wednesday 01 July 2015 05:37 PM, Frans Klaver wrote:
>> On Wed, Jul 1, 2015 at 1:56 PM, Sohny Thomas  wrote:
>>> - removed non-existant issue from TODO file
>>
>> s,existant,existent,
> Thanks missed that
>>
>>> kuid_t or uid_t not present in staging/android
>>> - fixed 80 char limit exceeding line
>>> - a newline after decelartions as per checkpatch.pl
>>> - fixed an unnecessary typedef as reported by checkpatch.pl
>>
>> Fix one issue per patch, please.
> Since these were all simple Fixes of about 1/2 lines , I thought to make a 
> single patch.

They are simple fixes, but a reviewer still has to figure out what
comment belongs to which code change. Since there's no reason for
these changes to be atomic, you might as well split them up to make
reviewing easier.

Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Staging: android: fix coding style and TODO file

[Frans Klaver]

On Wed, Jul 1, 2015 at 1:56 PM, Sohny Thomas  wrote:
> - removed non-existant issue from TODO file

s,existant,existent,

> kuid_t or uid_t not present in staging/android
> - fixed 80 char limit exceeding line
> - a newline after decelartions as per checkpatch.pl
> - fixed an unnecessary typedef as reported by checkpatch.pl

Fix one issue per patch, please.

Thanks,
Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2] lib/bitmap.c: fix some parsing issues and code style

[Frans Klaver]

On Wed, Jul 1, 2015 at 8:25 AM, Pan Xinhui  wrote:
> hello, Frans
> thanks for your reply :)
>
> On 2015年07月01日 14:17, Frans Klaver wrote:
>>
>> On Wed, Jul 1, 2015 at 6:15 AM, Pan Xinhui  wrote:
>>>
>>>
>>> In __bitmap_parselist we can accept whitespaces on head or tail
>>> during every parsing procedure.
>>> If input has valid ranges, there is no reason to reject the user.
>>>
>>> fixes are:
>>> 1) if input ends with ',', bit 0 might be set unexpectedly.
>>> now we check if any digit is available after every loop.
>>> 2) if input has '0-', bit 0 might be set unexpectedly,
>>> now we return -EINVAL as this kind of input is definitely wrong.
>>> 3) minor code style fix in __bitmap_parse.
>>> and avoid in-loop incrementation of ndigits.
>>
>>
>> Why not three patches, so it becomes easier to see which is which?
>>
> your advice sounds good, I will have a try. and welcome for review. :)
> thanks.
>
>>
>>> commit 2528a8b also add some check, but it's still not enough.
>>> it only correct the result in fix 1 above.
>>
>>
>> I believe the convention is to have at least 12 characters of the
>> sha1, with the title behind it: 2528a8b8f457 (__bitmap_parselist: fix
>> bug in empty string handling). Using only seven characters still risks
>> collisions.
>>
> sorry for my lack knowledge of comment rules. thanks for pointing out my
> mistakes.

No problem. These kinds of things can be caught if you use
scripts/checkpatch.pl on your patches before sending, by the way.

You should probably read Documentation/SubmittingPatches if you
haven't already done so.

Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2] lib/bitmap.c: fix some parsing issues and code style

[Frans Klaver]

On Wed, Jul 1, 2015 at 6:15 AM, Pan Xinhui  wrote:
>
> In __bitmap_parselist we can accept whitespaces on head or tail
> during every parsing procedure.
> If input has valid ranges, there is no reason to reject the user.
>
> fixes are:
> 1) if input ends with ',', bit 0 might be set unexpectedly.
> now we check if any digit is available after every loop.
> 2) if input has '0-', bit 0 might be set unexpectedly,
> now we return -EINVAL as this kind of input is definitely wrong.
> 3) minor code style fix in __bitmap_parse.
> and avoid in-loop incrementation of ndigits.

Why not three patches, so it becomes easier to see which is which?


> commit 2528a8b also add some check, but it's still not enough.
> it only correct the result in fix 1 above.

I believe the convention is to have at least 12 characters of the
sha1, with the title behind it: 2528a8b8f457 (__bitmap_parselist: fix
bug in empty string handling). Using only seven characters still risks
collisions.

Thanks,
Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND] rtl8712:Fix checkpatch warning

[Frans Klaver]

On Mon, Jun 29, 2015 at 10:18 PM, Ravi Teja Darbha  wrote:
>
> Fix line over 80 characters warning.
>
> Signed-off-by: Ravi Teja Darbha 
> ---
>  drivers/staging/rtl8712/rtl8712_recv.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/rtl8712/rtl8712_recv.c 
> b/drivers/staging/rtl8712/rtl8712_recv.c
> index fcb8c61..4fa2540 100644
> --- a/drivers/staging/rtl8712/rtl8712_recv.c
> +++ b/drivers/staging/rtl8712/rtl8712_recv.c
> @@ -58,8 +58,8 @@ int r8712_init_recv_priv(struct recv_priv *precvpriv, 
> struct _adapter *padapter)
>
>  /*init recv_buf*/
>  _init_queue(&precvpriv->free_recv_buf_queue);
> -precvpriv->pallocated_recv_buf = kzalloc(NR_RECVBUFF * sizeof(struct 
> recv_buf) + 4,
> - GFP_ATOMIC);
> +precvpriv->pallocated_recv_buf =
> +kzalloc(NR_RECVBUFF * sizeof(struct recv_buf) + 4, GFP_ATOMIC);
>  if (precvpriv->pallocated_recv_buf == NULL)
>  return _FAIL;
>  precvpriv->precv_buf = precvpriv->pallocated_recv_buf + 4 -
> --
> 1.9.1

Patch still got messed up. Try using git-send-email instead of
thunderbird. Or fix thunderbird not to mess with tab characters.

Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Stop SSD from waiting for "Spinning up disk..."

[Frans Klaver]

On Tue, Jun 23, 2015 at 5:02 PM, Jeff Chua  wrote:
> On Mon, Jun 22, 2015 at 11:36 PM, Greg Kroah-Hartman
>  wrote:
>> On Mon, Jun 22, 2015 at 03:25:29PM +0800, Jeff Chua wrote:
>>>
>>> There's no need to wait for disk spin-up for USB SSD devices. This patch
>>> allow the SSD to skip waiting disk spin-up by passing sd_mod.ssd=1 during
>>> boot-up.
>>>
>>> If there's a better way to handle this, please share.
>>
>> Module parameters are never a solution for a device-specific property,
>> sorry.
>
> Greg,
>
> SSD is coming mainstream and it doesn't make sense wasting time
> spinning up "disk" ...

I don't think Greg disputes that. He just objects to the solution (a
module parameter) you proposed. I'd guess that detecting that we're
talking to an SSD and then skipping the wait would make more sense, if
at all possible.

Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: rtl8192u: bool tests don't need comparisons

[Frans Klaver]

On Tue, Jun 23, 2015 at 3:59 PM, Luis de Bethencourt
 wrote:
> On Tue, Jun 23, 2015 at 03:37:20PM +0200, Frans Klaver wrote:
>> On Tue, Jun 23, 2015 at 3:21 PM, Luis de Bethencourt
>>  wrote:
>>
>> >> > if (dm_digtable.dig_algorithm_switch) {
>> >> > @@ -3062,7 +3062,8 @@ static void dm_dynamic_txpower(struct net_device 
>> >> > *dev)
>> >> > priv->bDynamicTxLowPower = false;
>> >> > } else {
>> >> > /* high power state check */
>> >> > -   if (priv->undecorated_smoothed_pwdb < 
>> >> > txlowpower_threshold && priv->bDynamicTxHighPower == true)
>> >> > +   if (priv->undecorated_smoothed_pwdb <
>> >> > +   txlowpower_threshold && 
>> >> > priv->bDynamicTxHighPower)
>> >> > priv->bDynamicTxHighPower = false;
>> >>
>> >> Oh, this has a misleading air hanging over it. It focuses the eyes on
>> >> "txlowpower_threshold && priv->bDynamicTxHighPower", while that
>> >> probably isn't the intent.
>> >>
>> >> Frans
>> >
>> > I agree, and wasn't sure what the best way to deal with was.
>> >
>> > The following doesn't mislead but goes above 80 characters.
>> > if (priv->undecorated_smoothed_pwdb < 
>> > txlowpower_threshold &&
>> > priv->bDynamicTxHighPower == true)
>> >
>> > It is better than the original but it doesn't completely fix it.
>> >
>> > If this is a better compromise I can update the patch.
>>
>> If we keep people's internal parsers working properly, I think having
>> a line of three characters too long is a fair compromise. Besides
>> that, there are a lot more lines of code in that file that need to be
>> brought back to under 80 characters.
>>
>> If you really care about that line length, precede with a patch (or
>> two) that changes those insanely long (local!) variable names, so that
>> you can break up the line right away.
>>
>> Have fun,
>> Frans
>
> Very true. There are a *lot* of massively long lines.
>
> This has been a learning experience. I wasn't sure how strict the rules for
> submissions were.

Well, as far as I know "Don't break internal parsers" wins over
"Checkpatch complains". However, checkpatch usually does have a nose
for smelly code (as does sparse, btw), so it pays to look around a bit
if it complains. In the end the maintainer decides whether a patch
passes the criteria.

> There are other things besides line lengths that I want to fix in
> rtl8192u. Related to that, I just sent a 3rd version which includes fixes for
> these bool comparisons for the rest of the files in drivers/staging/rtl8192u/
>
> Thanks so much for taking the time to review. Appreciated.

No problem. Was waiting for a yocto build to finish anyway.

Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/9] staging: vme_user: fix code alignment

[Frans Klaver]

On Tue, Jun 23, 2015 at 3:44 PM, Dmitry Kalinkin
 wrote:
>
>> On 23 Jun 2015, at 16:21, Frans Klaver  wrote:
>>
>> You left one in the function declarations (vme_user_write).
>
> If you mean forward declarations, they are already gone in Greg’s tree:
> https://git.kernel.org/cgit/linux/kernel/git/gregkh/staging.git/commit/drivers/staging/vme/devices/vme_user.c?h=staging-testing&id=e4aea6aa03267b496c21abefe170bb0d77192882

Yea, I meant those. Never mind then :)

Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: rtl8192u: bool tests don't need comparisons

[Frans Klaver]

On Tue, Jun 23, 2015 at 3:21 PM, Luis de Bethencourt
 wrote:

>> > if (dm_digtable.dig_algorithm_switch) {
>> > @@ -3062,7 +3062,8 @@ static void dm_dynamic_txpower(struct net_device 
>> > *dev)
>> > priv->bDynamicTxLowPower = false;
>> > } else {
>> > /* high power state check */
>> > -   if (priv->undecorated_smoothed_pwdb < 
>> > txlowpower_threshold && priv->bDynamicTxHighPower == true)
>> > +   if (priv->undecorated_smoothed_pwdb <
>> > +   txlowpower_threshold && 
>> > priv->bDynamicTxHighPower)
>> > priv->bDynamicTxHighPower = false;
>>
>> Oh, this has a misleading air hanging over it. It focuses the eyes on
>> "txlowpower_threshold && priv->bDynamicTxHighPower", while that
>> probably isn't the intent.
>>
>> Frans
>
> I agree, and wasn't sure what the best way to deal with was.
>
> The following doesn't mislead but goes above 80 characters.
> if (priv->undecorated_smoothed_pwdb < 
> txlowpower_threshold &&
> priv->bDynamicTxHighPower == true)
>
> It is better than the original but it doesn't completely fix it.
>
> If this is a better compromise I can update the patch.

If we keep people's internal parsers working properly, I think having
a line of three characters too long is a fair compromise. Besides
that, there are a lot more lines of code in that file that need to be
brought back to under 80 characters.

If you really care about that line length, precede with a patch (or
two) that changes those insanely long (local!) variable names, so that
you can break up the line right away.

Have fun,
Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/9] staging: vme_user: fix code alignment

[Frans Klaver]

On Tue, Jun 23, 2015 at 2:42 PM, Dmitry Kalinkin
 wrote:
> Signed-off-by: Dmitry Kalinkin 

You left one in the function declarations (vme_user_write).

> ---
>  drivers/staging/vme/devices/vme_user.c | 33 +
>  1 file changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/staging/vme/devices/vme_user.c 
> b/drivers/staging/vme/devices/vme_user.c
> index 5ff44fb..285e00e 100644
> --- a/drivers/staging/vme/devices/vme_user.c
> +++ b/drivers/staging/vme/devices/vme_user.c
> @@ -128,7 +128,7 @@ struct vme_user_vma_priv {
>   * transfer the data directly into the user space buffers.
>   */
>  static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
> -   loff_t *ppos)
> +   loff_t *ppos)
>  {
> ssize_t retval;
> ssize_t copied = 0;
> @@ -167,7 +167,7 @@ static ssize_t resource_to_user(int minor, char __user 
> *buf, size_t count,
>   * transfer the data directly from the user space buffers out to VME.
>   */
>  static ssize_t resource_from_user(unsigned int minor, const char __user *buf,
> -   size_t count, loff_t *ppos)
> + size_t count, loff_t *ppos)
>  {
> ssize_t retval;
> ssize_t copied = 0;
> @@ -195,7 +195,7 @@ static ssize_t resource_from_user(unsigned int minor, 
> const char __user *buf,
>  }
>
>  static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
> -   size_t count, loff_t *ppos)
> + size_t count, loff_t *ppos)
>  {
> void *image_ptr;
> ssize_t retval;
> @@ -214,7 +214,7 @@ static ssize_t buffer_to_user(unsigned int minor, char 
> __user *buf,
>  }
>
>  static ssize_t buffer_from_user(unsigned int minor, const char __user *buf,
> -   size_t count, loff_t *ppos)
> +   size_t count, loff_t *ppos)
>  {
> void *image_ptr;
> size_t retval;
> @@ -233,7 +233,7 @@ static ssize_t buffer_from_user(unsigned int minor, const 
> char __user *buf,
>  }
>
>  static ssize_t vme_user_read(struct file *file, char __user *buf, size_t 
> count,
> -   loff_t *ppos)
> +loff_t *ppos)
>  {
> unsigned int minor = MINOR(file_inode(file)->i_rdev);
> ssize_t retval;
> @@ -279,7 +279,7 @@ static ssize_t vme_user_read(struct file *file, char 
> __user *buf, size_t count,
>  }
>
>  static ssize_t vme_user_write(struct file *file, const char __user *buf,
> -   size_t count, loff_t *ppos)
> + size_t count, loff_t *ppos)
>  {
> unsigned int minor = MINOR(file_inode(file)->i_rdev);
> ssize_t retval;
> @@ -354,7 +354,7 @@ static loff_t vme_user_llseek(struct file *file, loff_t 
> off, int whence)
>   * already been defined.
>   */
>  static int vme_user_ioctl(struct inode *inode, struct file *file,
> -   unsigned int cmd, unsigned long arg)
> + unsigned int cmd, unsigned long arg)
>  {
> struct vme_master master;
> struct vme_slave slave;
> @@ -390,12 +390,13 @@ static int vme_user_ioctl(struct inode *inode, struct 
> file *file,
>  *  to userspace as they are
>  */
> retval = vme_master_get(image[minor].resource,
> -   &master.enable, &master.vme_addr,
> -   &master.size, &master.aspace,
> -   &master.cycle, &master.dwidth);
> +   &master.enable,
> +   &master.vme_addr,
> +   &master.size, &master.aspace,
> +   &master.cycle, 
> &master.dwidth);
>
> copied = copy_to_user(argp, &master,
> -   sizeof(struct vme_master));
> + sizeof(struct vme_master));
> if (copied != 0) {
> pr_warn("Partial copy to userspace\n");
> return -EFAULT;
> @@ -435,12 +436,12 @@ static int vme_user_ioctl(struct inode *inode, struct 
> file *file,
>  *  to userspace as they are
>  */
> retval = vme_slave_get(image[minor].resource,
> -   &slave.enable, &slave.vme_addr,
> -   &slave.size, &pci_addr, &slave.aspace,
> -   &slave.cycle);
> +  &slave.enable, &slave.vme_addr,
> +  &slave.size, &pci_addr,
> +  &slave.aspace, &slave.cycle);
>
> copied = copy_to_user(argp, &slave,
> -

Re: [PATCH] staging: rtl8192u: bool tests don't need comparisons

[Frans Klaver]

On Tue, Jun 23, 2015 at 2:52 PM, Luis de Bethencourt
 wrote:
> Remove explicit true/false comparations to bool variables.
>
> Signed-off-by: Luis de Bethencourt 
> ---
>  drivers/staging/rtl8192u/r8192U_core.c |  7 ---
>  drivers/staging/rtl8192u/r8192U_dm.c   | 21 +++--
>  2 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/rtl8192u/r8192U_core.c 
> b/drivers/staging/rtl8192u/r8192U_core.c
> index a4795af..c53d670 100644
> --- a/drivers/staging/rtl8192u/r8192U_core.c
> +++ b/drivers/staging/rtl8192u/r8192U_core.c
> @@ -2047,7 +2047,7 @@ static bool GetHalfNmodeSupportByAPs819xUsb(struct 
> net_device *dev)
> struct r8192_priv *priv = ieee80211_priv(dev);
> struct ieee80211_device *ieee = priv->ieee80211;
>
> -   if (ieee->bHalfWirelessN24GMode == true)
> +   if (ieee->bHalfWirelessN24GMode)
> Reval = true;
> else
> Reval =  false;
> @@ -2762,7 +2762,7 @@ static bool rtl8192_adapter_start(struct net_device 
> *dev)
> //
>  #ifdef TO_DO_LIST
> if (Adapter->ResetProgress == RESET_TYPE_NORESET) {
> -   if (pMgntInfo->RegRfOff == true) { /* User disable RF via 
> registry. */
> +   if (pMgntInfo->RegRfOff) { /* User disable RF via registry. */
> RT_TRACE((COMP_INIT|COMP_RF), DBG_LOUD, 
> ("InitializeAdapter819xUsb(): Turn off RF for RegRfOff --\n"));
> MgntActSet_RF_State(Adapter, eRfOff, RF_CHANGE_BY_SW);
> // Those actions will be discard in 
> MgntActSet_RF_State because of the same state
> @@ -4406,7 +4406,8 @@ static void query_rxdesc_status(struct sk_buff *skb,
> /* RTL8190 set this bit to indicate that Hw does not decrypt packet */
> stats->Decrypted = !desc->SWDec;
>
> -   if ((priv->ieee80211->pHTInfo->bCurrentHTSupport == true) && 
> (priv->ieee80211->pairwise_key_type == KEY_TYPE_CCMP))
> +   if ((priv->ieee80211->pHTInfo->bCurrentHTSupport) &&
> +   (priv->ieee80211->pairwise_key_type == KEY_TYPE_CCMP))
> stats->bHwError = false;
> else
> stats->bHwError = stats->bCRC|stats->bICV;
> diff --git a/drivers/staging/rtl8192u/r8192U_dm.c 
> b/drivers/staging/rtl8192u/r8192U_dm.c
> index 12dd19e..9946615 100644
> --- a/drivers/staging/rtl8192u/r8192U_dm.c
> +++ b/drivers/staging/rtl8192u/r8192U_dm.c
> @@ -438,7 +438,7 @@ static void dm_bandwidth_autoswitch(struct net_device 
> *dev)
>
> if (priv->CurrentChannelBW == HT_CHANNEL_WIDTH_20 || 
> !priv->ieee80211->bandwidth_auto_switch.bautoswitch_enable)
> return;
> -   if (priv->ieee80211->bandwidth_auto_switch.bforced_tx20Mhz == false) 
> { /* If send packets in 40 Mhz in 20/40 */
> +   if (!priv->ieee80211->bandwidth_auto_switch.bforced_tx20Mhz) { /* If 
> send packets in 40 Mhz in 20/40 */
> if (priv->undecorated_smoothed_pwdb <= 
> priv->ieee80211->bandwidth_auto_switch.threshold_40Mhzto20Mhz)
> 
> priv->ieee80211->bandwidth_auto_switch.bforced_tx20Mhz = true;
> } else { /* in force send packets in 20 Mhz in 20/40 */
> @@ -563,7 +563,7 @@ static void dm_TXPowerTrackingCallback_TSSI(struct 
> net_device *dev)
> break;
> }
> }
> -   if (viviflag == true) {
> +   if (viviflag) {
> write_nic_byte(dev, 0x1ba, 0);
> viviflag = false;
> RT_TRACE(COMP_POWER_TRACKING, "we filtered 
> the data\n");
> @@ -766,7 +766,7 @@ void dm_txpower_trackingcallback(struct work_struct *work)
> struct r8192_priv *priv = container_of(dwork, struct r8192_priv, 
> txpower_tracking_wq);
> struct net_device *dev = priv->ieee80211->dev;
>
> -   if (priv->bDcut == true)
> +   if (priv->bDcut)
> dm_TXPowerTrackingCallback_TSSI(dev);
> else
> dm_TXPowerTrackingCallback_ThermalMeter(dev);
> @@ -1301,7 +1301,7 @@ void dm_initialize_txpower_tracking(struct net_device 
> *dev)
>  {
> struct r8192_priv *priv = ieee80211_priv(dev);
>
> -   if (priv->bDcut == true)
> +   if (priv->bDcut)
> dm_InitializeTXPowerTracking_TSSI(dev);
> else
> dm_InitializeTXPowerTracking_ThermalMeter(dev);
> @@ -1357,7 +1357,7 @@ static void dm_check_txpower_tracking(struct net_device 
> *dev)
>  #ifdef RTL8190P
> dm_CheckTXPowerTracking_TSSI(dev);
>  #else
> -   if (priv->bDcut == true)
> +   if (priv->bDcut)
> dm_CheckTXPowerTracking_TSSI(dev);
> else
> dm_CheckTXPowerTracking_ThermalMeter(dev);
> @@ -1467,7 +1467,7 @@ void dm_cck_txpower_adjust(struct net_device *dev, bool 
> binch14)
>  {  /*  dm_CCKTxPowerAdjust */
> s

Re: [PATCH] staging: rtl8192u: bool tests don't need comparisons

[Frans Klaver]

On Tue, Jun 23, 2015 at 2:52 PM, Luis de Bethencourt
 wrote:
> Remove explicit true/false comparations to bool variables.
>
> Signed-off-by: Luis de Bethencourt 
> ---
>  drivers/staging/rtl8192u/r8192U_core.c |  7 ---
>  drivers/staging/rtl8192u/r8192U_dm.c   | 21 +++--
>  2 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/rtl8192u/r8192U_core.c 
> b/drivers/staging/rtl8192u/r8192U_core.c
> index a4795af..c53d670 100644
> --- a/drivers/staging/rtl8192u/r8192U_core.c
> +++ b/drivers/staging/rtl8192u/r8192U_core.c
> @@ -2047,7 +2047,7 @@ static bool GetHalfNmodeSupportByAPs819xUsb(struct 
> net_device *dev)
> struct r8192_priv *priv = ieee80211_priv(dev);
> struct ieee80211_device *ieee = priv->ieee80211;
>
> -   if (ieee->bHalfWirelessN24GMode == true)
> +   if (ieee->bHalfWirelessN24GMode)
> Reval = true;
> else
> Reval =  false;

With this one I'd go as far as saying that

Reval = ieee->bHalfWirelessN24GMode;

Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH v2] packet: remove handling of tx_ring

[Frans Klaver]

Hi,

On Mon, Jun 22, 2015 at 8:53 AM, Maninder Singh  wrote:
> Hi Frans,
>
>>> v1 = replace if()/BUG with BUG_ON() for tx_ring.
>>>
>>> v2 =
>>
>>I would keep this below the ---. There's little historical use for
>>this version information when it gets merged.
>>
>>> remove handling of tx_ring in prb_setup_retire_blk_timer
>>> for TPACKET_V3 because init_prb_bdqc is called only for NULL tx_ring
>>> and thus prb_setup_retire_blk_timer for NULL tx_ring only.
>>
>>I'd say tx_ring is false, rather than NULL. It's not a pointer (here).
>>
>>
>>> And also in funciton init_prb_bdqc there is no usage of tx_ring.
>>
>>s,funciton,function,
>
> Thanks for feedback , please check below changelog if it looks ok,
> Then i will share updated patch:-
>
> v1 = replace if()/BUG with BUG_ON() for tx_ring.
>
> Signed-off-by: Maninder Singh 
> Signed-off-by: Frans Klaver 

No, the Suggested-by: was better for me. You can't go about and add
Signed-off-by lines for someone else without permission ;-).

> ---
> Changes in v2:
>
>  Remove handling of tx_ring in prb_setup_retire_blk_timer
>  for TPACKET_V3 because init_prb_bdqc is called only for zero tx_ring
>  and thus prb_setup_retire_blk_timer for zero tx_ring only.
>
>  And also in functon init_prb_bdqc there is no usage of tx_ring.
>  Thus removing tx_ring from init_prb_bdqc.
>
>  net/packet/af_packet.c |   14 +-
>  1 file changed, 5 insertions(+), 9 deletions(-)
>
>  Thanks
>  Maninder

No, I didn't make myself clear enough, I'm afraid. The info about the
different incarnations of your patch should go below the dashes. The
whole "Remove handling " text should be your commit message,
because that is what you want to see in the commit log. Here's an
example:

Subject: [RFC PATCH v2] packet: remove handling of tx_ring

Remove handling of tx_ring in prb_setup_retire_blk_timer for
TPACKET_V3 because init_prb_bdqc is called only for zero tx_ring and
thus prb_setup_retire_blk_timer for zero tx_ring only.

And also in functon init_prb_bdqc there is no usage of tx_ring. Thus
removing tx_ring from init_prb_bdqc.

Signed-off-by: Maninder Singh 
Suggested-by: Frans Klaver 
---
v1..v2: remove BUG() by removing tx_path

diffstat & patch
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH v2] packet: remove handling of tx_ring

[Frans Klaver]

On Mon, Jun 22, 2015 at 7:54 AM, Maninder Singh  wrote:
> v1 = replace if()/BUG with BUG_ON() for tx_ring.
>
> v2 =

I would keep this below the ---. There's little historical use for
this version information when it gets merged.

> remove handling of tx_ring in prb_setup_retire_blk_timer
> for TPACKET_V3 because init_prb_bdqc is called only for NULL tx_ring
> and thus prb_setup_retire_blk_timer for NULL tx_ring only.

I'd say tx_ring is false, rather than NULL. It's not a pointer (here).


> And also in funciton init_prb_bdqc there is no usage of tx_ring.

s,funciton,function,


> Thus removing tx_ring from init_prb_bdqc.
>
> Signed-off-by: Maninder Singh 
> Suggested-by: Frans Klaver 
> ---
>  net/packet/af_packet.c |   14 +-
>  1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index fd51641..aeafcf0 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -543,15 +543,11 @@ static void prb_init_blk_timer(struct packet_sock *po,
> pkc->retire_blk_timer.expires = jiffies;
>  }
>
> -static void prb_setup_retire_blk_timer(struct packet_sock *po, int tx_ring)
> +static void prb_setup_retire_blk_timer(struct packet_sock *po)
>  {
> struct tpacket_kbdq_core *pkc;
>
> -   if (tx_ring)
> -   BUG();
> -
> -   pkc = tx_ring ? GET_PBDQC_FROM_RB(&po->tx_ring) :
> -   GET_PBDQC_FROM_RB(&po->rx_ring);
> +   pkc = GET_PBDQC_FROM_RB(&po->rx_ring);
> prb_init_blk_timer(po, pkc, prb_retire_rx_blk_timer_expired);
>  }
>
> @@ -607,7 +603,7 @@ static void prb_init_ft_ops(struct tpacket_kbdq_core *p1,
>  static void init_prb_bdqc(struct packet_sock *po,
> struct packet_ring_buffer *rb,
> struct pgv *pg_vec,
> -   union tpacket_req_u *req_u, int tx_ring)
> +   union tpacket_req_u *req_u)
>  {
> struct tpacket_kbdq_core *p1 = GET_PBDQC_FROM_RB(rb);
> struct tpacket_block_desc *pbd;
> @@ -634,7 +630,7 @@ static void init_prb_bdqc(struct packet_sock *po,
>
> p1->max_frame_len = p1->kblk_size - 
> BLK_PLUS_PRIV(p1->blk_sizeof_priv);
> prb_init_ft_ops(p1, req_u);
> -   prb_setup_retire_blk_timer(po, tx_ring);
> +   prb_setup_retire_blk_timer(po);
> prb_open_block(p1, pbd);
>  }
>
> @@ -4001,7 +3997,7 @@ static int packet_set_ring(struct sock *sk, union 
> tpacket_req_u *req_u,
>  * it above but just being paranoid
>  */
> if (!tx_ring)
> -   init_prb_bdqc(po, rb, pg_vec, req_u, tx_ring);
> +   init_prb_bdqc(po, rb, pg_vec, req_u);
> break;
> default:
> break;
> --
> 1.7.9.5

Looks like what I made myself yesterday evening.

Thanks,
Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: Coding style details (checkpatch)

[Frans Klaver]

On Fri, Jun 19, 2015 at 12:31 PM, Krzysztof Hałasa  wrote:
> Frans Klaver  writes:
>
>>> #define REG8_1(a0) ((const u16[8]){a0, a0 + 1, a0 + 2, a0 + 3})
>>>
>>> vs
>>>
>>> #define REG8_1(a0) ((const u16[8]) {a0, a0 + 1, a0 + 2, a0 + 3})
>>> ^
>>
>> The prescribed style is to have no space between cast and castee. So,
>> the top option.
>
> Thanks. That's what I thought. It looks that checkpatch doesn't like
> this:
>
> ERROR: space required before the open brace '{'
> +#define REG8_1(a0) ((const u16[8]){a0, a0 + 1, a0 + 2, a0 + 3})
>
> Does this qualify as the "false positive"?

Ah, right. One might say that this is a false positive, but that's up
to Joe or Andy I guess.

It may be valid C code, but I think this construction is slightly
funky to begin with.

Which file is this?

Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Coding style details

[Frans Klaver]

Hi,

On Fri, Jun 19, 2015 at 10:35 AM, Krzysztof Hałasa  wrote:
> Hi,
>
> a simple question: which style is preferred?
>
> #define REG8_1(a0) ((const u16[8]){a0, a0 + 1, a0 + 2, a0 + 3})
>
> vs
>
> #define REG8_1(a0) ((const u16[8]) {a0, a0 + 1, a0 + 2, a0 + 3})
> ^

The prescribed style is to have no space between cast and castee. So,
the top option.

Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 01/44] kernel: Add support for poweroff handler call chain

[Frans Klaver]

On Thu, Jun 18, 2015 at 1:54 PM, Guenter Roeck  wrote:
> On 06/17/2015 11:53 PM, Frans Klaver wrote:
>>
>> On Thu, Jun 18, 2015 at 3:04 AM, Stephen Boyd 
>> wrote:
>>>
>>> On 10/06/2014 10:28 PM, Guenter Roeck wrote:
>>>>
>>>> Various drivers implement architecture and/or device specific means to
>>>> remove power from the system.  For the most part, those drivers set the
>>>> global variable pm_power_off to point to a function within the driver.
>>>>
>>>> This mechanism has a number of drawbacks.  Typically only one scheme
>>>> to remove power is supported (at least if pm_power_off is used).
>>>> At least in theory there can be multiple means remove power, some of
>>>> which may be less desirable. For example, some mechanisms may only
>>>> power off the CPU or the CPU card, while another may power off the
>>>> entire system.  Others may really just execute a restart sequence
>>>> or drop into the ROM monitor. Using pm_power_off can also be racy
>>>> if the function pointer is set from a driver built as module, as the
>>>> driver may be in the process of being unloaded when pm_power_off is
>>>> called. If there are multiple poweroff handlers in the system, removing
>>>> a module with such a handler may inadvertently reset the pointer to
>>>> pm_power_off to NULL, leaving the system with no means to remove power.
>>>>
>>>> Introduce a system poweroff handler call chain to solve the described
>>>> problems.  This call chain is expected to be executed from the
>>>> architecture specific machine_power_off() function.  Drivers providing
>>>> system poweroff functionality are expected to register with this call
>>>> chain.
>>>> By using the priority field in the notifier block, callers can control
>>>> poweroff handler execution sequence and thus ensure that the poweroff
>>>> handler with the optimal capabilities to remove power for a given system
>>>> is called first.
>>>
>>>
>>> What happened to this series? I want to add shutdown support to my
>>> platform and I need to write a register on the PMIC in one driver to
>>> configure it for shutdown instead of restart and then write an MMIO
>>> register to tell the PMIC to actually do the shutdown in another driver.
>>> It seems that the notifier solves this case for me, albeit with the
>>> slight complication that I need to order the two with some priority.
>>
>>
>> I was wondering the same thing. I did find out that things kind of
>> stalled after Linus cast doubt on the chosen path [1]. I'm not sure
>> there's any consensus on what would be best to do instead.
>>
>
> Linus cast doubt on it, then the maintainers started picking it apart.
> At the end, trying not to use notifier callbacks made the code so
> complicated that even I didn't understand it anymore. With no consensus
> in sight, I abandoned it.
>
> Problem is really that the notifier call chain would be perfect to solve
> the problem, yet Linus didn't like priorities (which are essential),
> and the power maintainers didn't like that a call chain is supposed
> to execute _all_ callbacks, which would not be the case here. If I were
> to start again, I would insist to use notifiers. However, I don't see
> a chance to get that accepted, so I won't. Feel free to pick it up and
> give it a try yourself.

How about having two phases? One where all interested parts of the
system get notified, one that does the final shutdown. It's a slightly
different approach than you took, but does use the notifier chains as
expected, and can be used to prepare peripherals for shutdown, if
there's a use case for it.

The two-stage approach does keep the single place to power down. I
expect it would become more obvious that it would be silly to have
more than one actual system power down sequence and hiding
pm_power_off and unifying setting of it should become more straight
forward as well.

Thoughts?

Thanks,
Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/