Re: [PATCH v3 1/4] iio: adc: Add support for AD7091R5 ADC

2019-11-08 Thread Dan Carpenter
On Thu, Nov 07, 2019 at 05:07:56PM +0200, Beniamin Bia wrote:
> +static int ad7091r_set_mode(struct ad7091r_state *st, enum ad7091r_mode mode)
> +{
> + int ret, conf;
> +
> + switch (mode) {
> + case AD7091R_MODE_SAMPLE:
> + conf = 0;
> + break;
> + case AD7091R_MODE_COMMAND:
> + conf = AD7091R_REG_CONF_CMD;
> + break;
> + case AD7091R_MODE_AUTOCYCLE:
> + conf = AD7091R_REG_CONF_AUTO;
> + break;
> + default:
> + ret = -EINVAL;
> + break;

return -EINVAL;

> + }
> +
> + ret = regmap_update_bits(st->map, AD7091R_REG_CONF,
> +  AD7091R_REG_CONF_MODE_MASK, conf);


otherwise conf is uninitialized.

> + if (ret)
> + return ret;
> +
> + st->mode = mode;
> +
> + return ret;

return 0;

> +}
> +
> +static int ad7091r_set_channel(struct ad7091r_state *st, unsigned int 
> channel)
> +{
> + unsigned int foo;

Use unsigned int dummy.

> + int ret;
> +

Otherwise it looks ok to me.  (Not a domain expert).

regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: octeon: fix missing a blank line after declaration

2019-11-07 Thread Dan Carpenter
On Thu, Nov 07, 2019 at 02:13:22PM +, Valery Ivanov wrote:
>   This patch fixes "WARNING: Missing a blank line after declarations"
>   Issue found by checkpatch.pl

I think maybe you are cutting and pasting from git log output or
something?  Anyway, please don't indent your commit message like this.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] mtd: rawnand: driver for Mediatek MT7621 SoC NAND flash controller

2019-11-07 Thread Dan Carpenter
return -ERANGE;
> + oobregion->offset = layout->oobfree[section].offset;
> + oobregion->length = layout->oobfree[section].length;
> + return 0;
> +}
> +
> +/*
> + * Code to support the legacy mediatek nand flash bad block table.
> + * The default for this driver is to use the standard Linux bad block
> + * table format. However you need a new boot loader that supports that.
> + * The old (and most often used) medaitek boot loaders use their own
> + * BBT format, and this code implements that. There is a devicetree
> + * binding that en

Re: [PATCH] staging: rtl8192e: fix potential use after free

2019-11-05 Thread Dan Carpenter
On Tue, Nov 05, 2019 at 10:49:11PM +0800, Pan Bian wrote:
> The variable skb is released via kfree_skb() when the return value of
> _rtl92e_tx is not zero. However, after that, skb is accessed again to
> read its length, which may result in a use after free bug. This patch
> fixes the bug by moving the release operation to where skb is never
> used later.
> 
> Signed-off-by: Pan Bian 

Reviewed-by: Dan Carpenter 

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2] hp100: remove set but not used variable val

2019-11-05 Thread Dan Carpenter
On Tue, Nov 05, 2019 at 10:36:59PM +0800, Chen Wandun wrote:
> From: Chenwandun 
> 
> Fixes gcc '-Wunused-but-set-variable' warning:
> 
> drivers/staging/hp/hp100.c: In function hp100_start_xmit:
> drivers/staging/hp/hp100.c:1629:10: warning: variable val set but not used 
> [-Wunused-but-set-variable]
> 
> Signed-off-by: Chenwandun 
> ---

You should explain why you are sending a v2 under the --- cut off line.

The v1 was the correct patch, but this driver is going to be deleted
soon so I don't think we are accepting this sort of cleanup.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2] staging: wfx: Fix a memory leak in 'wfx_upload_beacon'

2019-11-04 Thread Dan Carpenter
On Mon, Nov 04, 2019 at 10:42:43AM +, Jerome Pouiller wrote:
> On Saturday 2 November 2019 16:59:45 CET Christophe JAILLET wrote:
> > The current code is a no-op, because all it can do is 'dev_kfree_skb(NULL)'
> > Remove the test before 'dev_kfree_skb()'
> > 
> > Signed-off-by: Christophe JAILLET 
> > ---
> > V2: remove the 'if(...)', 'dev_kfree_skb()' can handle NULL.
> > ---
> >  drivers/staging/wfx/sta.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
> > index 688586e823c0..93f3739b5f3a 100644
> > --- a/drivers/staging/wfx/sta.c
> > +++ b/drivers/staging/wfx/sta.c
> > @@ -906,8 +906,7 @@ static int wfx_upload_beacon(struct wfx_vif *wvif)
> > wfx_fwd_probe_req(wvif, false);
> > 
> >  done:
> > -   if (!skb)
> > -   dev_kfree_skb(skb);
> > +   dev_kfree_skb(skb);
> > return ret;
> >  }
> > 
> > --
> > 2.20.1
> > 
> 
> In add, value of skb is tested earlier in function. So, it is guaranteed to 
> be 
> never NULL.

See the start of the function:

   868  static int wfx_upload_beacon(struct wfx_vif *wvif)
   869  {
   870  int ret = 0;
   871  struct sk_buff *skb = NULL;
   872  struct ieee80211_mgmt *mgmt;
   873  struct hif_mib_template_frame *p;
   874  
   875  if (wvif->vif->type == NL80211_IFTYPE_STATION ||
   876  wvif->vif->type == NL80211_IFTYPE_MONITOR ||
   877  wvif->vif->type == NL80211_IFTYPE_UNSPECIFIED)
   878  goto done;
^
This why I argue that goto done is monkey poop.  You see this and you
wonder "what on earth does goto done do?"  We haven't allocated anything
so what are we going to free?  The name doesn't give any hints at all.
So you have to scroll down.  Now you have lost your place and cleared
out your short term memory about the code that you were reading.  But
then you get to goto done and you see it is a complicated no op, and it
return ret.  So you have to scroll back up to the very start of the
function to see what ret is.

And the you wonder, is this really a success path?  It could be
deliberate or it could be accidental.  Who knows!  The "goto done;" is
ambiguous and take a long time to understand but "return 0;" is
instantly clear.

   879  
   880      skb = ieee80211_beacon_get(wvif->wdev->hw, wvif->vif);
   881  
   882  if (!skb)
   883  return -ENOMEM;
   884  
   885  p = (struct hif_mib_template_frame *) skb_push(skb, 4);

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 07/10] staging: exfat: Clean up return codes - FFS_SUCCESS

2019-11-04 Thread Dan Carpenter
On Mon, Nov 04, 2019 at 05:53:55AM -0500, Valdis Klētnieks wrote:
> On Mon, 04 Nov 2019 13:04:14 +0300, Dan Carpenter said:
> > On Sun, Nov 03, 2019 at 08:45:03PM -0500, Valdis Kletnieks wrote:
> > > - if (sector_read(sb, sec, >buf_bh, 1) != FFS_SUCCESS) {
> > > + if (sector_read(sb, sec, >buf_bh, 1) != 0) {
> >
> > It's better to just remove the "!= 0" double negative.  != 0 should be
> > used when we are talking about the number zero as in "cnt != 0" and for
> > "strcmp(foo, bar) != 0" where it means that "foo != bar".
> 
> "Fix up ==0 and !=0" is indeed on the to-do list.
> 
> This patch converted 82 uses of FFS_SUCCESS, of which 33 had the != idiom in
> use.  Meanwhile, overall there's 53 '!= 0' and 95 '== 0' uses.
> 
> In other words, even if I fixed all of those that were involved in this patch,
> there would *still* be more patching to do.

Very good.  Sounds like the plan.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 07/10] staging: exfat: Clean up return codes - FFS_SUCCESS

2019-11-04 Thread Dan Carpenter
On Sun, Nov 03, 2019 at 08:45:03PM -0500, Valdis Kletnieks wrote:
> diff --git a/drivers/staging/exfat/exfat_cache.c 
> b/drivers/staging/exfat/exfat_cache.c
> index 467b93630d86..28a67f8139ea 100644
> --- a/drivers/staging/exfat/exfat_cache.c
> +++ b/drivers/staging/exfat/exfat_cache.c
> @@ -462,7 +462,7 @@ u8 *FAT_getblk(struct super_block *sb, sector_t sec)
>  
>   FAT_cache_insert_hash(sb, bp);
>  
> - if (sector_read(sb, sec, >buf_bh, 1) != FFS_SUCCESS) {
> + if (sector_read(sb, sec, >buf_bh, 1) != 0) {

It's better to just remove the "!= 0" double negative.  != 0 should be
used when we are talking about the number zero as in "cnt != 0" and for
"strcmp(foo, bar) != 0" where it means that "foo != bar".

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: rtl8192u: fix potential infinite loop because loop counter being too small

2019-11-01 Thread Dan Carpenter
On Fri, Nov 01, 2019 at 02:26:04PM +, Colin King wrote:
> From: Colin Ian King 
> 
> Currently the for-loop counter i is a u8 however it is being checked
> against a maximum value priv->ieee80211->LinkDetectInfo.SlotNum which is a
> u16. Hence there is a potential wrap-around of counter i back to zero if
> priv->ieee80211->LinkDetectInfo.SlotNum is greater than 255.  Fix this by
> making i a u16.
> 
> Addresses-Coverity: ("Infinite loop")
> Fixes: 8fc8598e61f6 ("Staging: Added Realtek rtl8192u driver to staging")
> Signed-off-by: Colin Ian King 
> ---
>  drivers/staging/rtl8192u/r8192U_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rtl8192u/r8192U_core.c 
> b/drivers/staging/rtl8192u/r8192U_core.c
> index 48f1591ed5b4..fd91b7c5ca81 100644
> --- a/drivers/staging/rtl8192u/r8192U_core.c
> +++ b/drivers/staging/rtl8192u/r8192U_core.c
> @@ -3210,7 +3210,7 @@ static void rtl819x_update_rxcounts(struct r8192_priv 
> *priv, u32 *TotalRxBcnNum,
>u32 *TotalRxDataNum)
>  {
>   u16 SlotIndex;
> - u8  i;
> + u16     i;

The iterator "i" should just be an int unless we know that it needs to
be an unsigned long long.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 6/6] staging: exfat: replace kmalloc with kmalloc_array

2019-11-01 Thread Dan Carpenter
On Thu, Oct 31, 2019 at 05:03:56PM +0100, Roi Martin wrote:
> > > diff --git a/drivers/staging/exfat/exfat_core.c 
> > > b/drivers/staging/exfat/exfat_core.c
> > > index f71235c6a338..f4f82aecc05d 100644
> > > --- a/drivers/staging/exfat/exfat_core.c
> > > +++ b/drivers/staging/exfat/exfat_core.c
> > > @@ -713,8 +713,8 @@ static s32 __load_upcase_table(struct super_block 
> > > *sb, sector_t sector,
> > >  
> > >   u32 checksum = 0;
> > >  
> > > - upcase_table = p_fs->vol_utbl = kmalloc(UTBL_COL_COUNT * sizeof(u16 *),
> > > - GFP_KERNEL);
> > > + upcase_table = kmalloc_array(UTBL_COL_COUNT, sizeof(u16 *), GFP_KERNEL);
> > > + p_fs->vol_utbl = upcase_table;
> > 
> > This patch is fine, but one idea for future patches is that you could
> > remove the "upcase_table" variable and use "p_fs->vol_utbl" everywhere
> > instead.
> 
> Thanks for the suggestion.
> 
> This is my first contribution and I tried to introduce the minimum
> number of changes necessary to fix the issues reported by checkpatch.pl.
> Also, I'm still immersed in getting familiar with the contribution
> process and the code.
> 
> Do you think it makes sense to include this change in a future patch
> series along with other refactoring? Or, should I modify this patch?

No don't modify the patch.  The patch is fine.

> 
> By the way, upcase_table is sometimes accessed in quite complex ways.
> For instance:
> 
>   upcase_table[col_index][get_row_index(index)] = uni;
> 
> Where having an intermediate variable instead of using the struct field
> directly seems to improve readability a bit. Otherwise:
> 
>   p_fs->vol_utbl[col_index][get_row_index(index)] = uni;

This line isn't very complex.  It's fine.


> 
> I assume, in cases like this, from a coding style perspective, the
> following approach is preferred:
> 
>   row_index = get_row_index(index);
>   p_fs->vol_utbl[col_index][row_index] = uni;

But this is better, yes.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 6/6] staging: exfat: replace kmalloc with kmalloc_array

2019-10-31 Thread Dan Carpenter
On Thu, Oct 31, 2019 at 01:31:39PM +0100, Roi Martin wrote:
> diff --git a/drivers/staging/exfat/exfat_core.c 
> b/drivers/staging/exfat/exfat_core.c
> index f71235c6a338..f4f82aecc05d 100644
> --- a/drivers/staging/exfat/exfat_core.c
> +++ b/drivers/staging/exfat/exfat_core.c
> @@ -713,8 +713,8 @@ static s32 __load_upcase_table(struct super_block *sb, 
> sector_t sector,
>  
>   u32 checksum = 0;
>  
> - upcase_table = p_fs->vol_utbl = kmalloc(UTBL_COL_COUNT * sizeof(u16 *),
> - GFP_KERNEL);
> + upcase_table = kmalloc_array(UTBL_COL_COUNT, sizeof(u16 *), GFP_KERNEL);
> + p_fs->vol_utbl = upcase_table;

This patch is fine, but one idea for future patches is that you could
remove the "upcase_table" variable and use "p_fs->vol_utbl" everywhere
instead.

>   if (!upcase_table)
>   return -ENOMEM;

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 1/4] iio: adc: Add support for AD7091R5 ADC

2019-10-29 Thread Dan Carpenter
It looks like you're going to have to respin the patchset so I'm adding
my nits even though it's a bit late.


> +static int ad7091r_set_mode(struct ad7091r_state *st, enum ad7091r_mode mode)
> +{
> + int ret;
> +
> + switch (mode) {
> + case AD7091R_MODE_SAMPLE:
> + ret = regmap_update_bits(st->map, AD7091R_REG_CONF,
> +  AD7091R_REG_CONF_MODE_MASK, 0);
> + break;
> + case AD7091R_MODE_COMMAND:
> + ret = regmap_update_bits(st->map, AD7091R_REG_CONF,
> +  AD7091R_REG_CONF_MODE_MASK,
> +  AD7091R_REG_CONF_CMD);
> + break;
> + case AD7091R_MODE_AUTOCYCLE:
> + ret = regmap_update_bits(st->map, AD7091R_REG_CONF,
> +  AD7091R_REG_CONF_MODE_MASK,
> +  AD7091R_REG_CONF_AUTO);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }

This would look even better as:

switch (mode) {
case AD7091R_MODE_SAMPLE:
conf = 0;
break;
case AD7091R_MODE_COMMAND:
conf = AD7091R_REG_CONF_CMD;
break;
case AD7091R_MODE_AUTOCYCLE:
conf = AD7091R_REG_CONF_AUTO;
break;
default:
return -EINVAL;
}

ret = regmap_update_bits(st->map, AD7091R_REG_CONF,
 AD7091R_REG_CONF_MODE_MASK, conf);
if (ret)
return ret;

st->mode = mode;

return 0;

> +int ad7091r_probe(struct device *dev, const char *name,
> + const struct ad7091r_chip_info *chip_info,
> + struct regmap *map, int irq)
> +{
> + struct iio_dev *iio_dev;
> + struct ad7091r_state *st;
> + int ret;
> +
> + iio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> + if (!iio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(iio_dev);
> + st->dev = dev;
> + st->chip_info = chip_info;
> + st->map = map;
> +
> + iio_dev->dev.parent = dev;
> + iio_dev->name = name;
> + iio_dev->info = _info;
> + iio_dev->modes = INDIO_DIRECT_MODE;
> +
> + iio_dev->num_channels = chip_info->num_channels;
> + iio_dev->channels = chip_info->channels;
> +
> + if (irq) {
> + ret = devm_request_threaded_irq(dev, irq, NULL,
> + ad7091r_event_handler,
> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, name, st);
> + if (ret)
> + return ret;
> + }
> +
> + /* Use command mode by default to convert only desired channels*/
> + ret = ad7091r_set_mode(st, AD7091R_MODE_COMMAND);
> + if (ret < 0)

if (ret) {

> + return ret;
> +
> + return iio_device_register(iio_dev);
> +}
> +EXPORT_SYMBOL_GPL(ad7091r_probe);

[ snip ]

> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "ad7091r-base.h"
> +
> +static const struct iio_event_spec ad7091r5_events[] = {
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_RISING,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> + BIT(IIO_EV_INFO_ENABLE),

This would be more clear if it were aligned like so:

.mask_separate = BIT(IIO_EV_INFO_VALUE) |
 BIT(IIO_EV_INFO_ENABLE),


> + },
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_FALLING,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> + BIT(IIO_EV_INFO_ENABLE),

        .mask_separate = BIT(IIO_EV_INFO_VALUE) |
 BIT(IIO_EV_INFO_ENABLE),

> + },
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_EITHER,
> + .mask_separate = BIT(IIO_EV_INFO_HYSTERESIS),
> + },
> +};
> +

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: vc04_services: replace g_free_fragments_mutex with spinlock

2019-10-28 Thread Dan Carpenter
On Mon, Oct 28, 2019 at 09:35:37AM -0700, Davidlohr Bueso wrote:
> On Mon, 28 Oct 2019, Greg KH wrote:
> > This is obviously not in a format I can apply it in :(
> 
> What are you talking about? I sent you the original patch,
> then Cc'ed the drivers mailing list.  So you still have a
> patch you can apply... this is quite a common way of doing
> things (Ccing for future references to someone or another
> ml). I don't understand why you are hairsplitting over this
> patch.
> 

I don't have the original patch either.  Only the corrupted one...  Maybe
you did it as html and it was rejected?

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v3 1/2] staging: rtl8712: Fix Alignment of open parenthesis

2019-10-28 Thread Dan Carpenter
On Mon, Oct 28, 2019 at 12:15:52PM -0300, Cristiane Naves wrote:
> Fix alignment should match open parenthesis. Issue found by checkpatch.
> 
> Signed-off-by: Cristiane Naves 
> ---
>  drivers/staging/rtl8712/rtl8712_recv.c | 36 
> +-
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/staging/rtl8712/rtl8712_recv.c 
> b/drivers/staging/rtl8712/rtl8712_recv.c
> index af12c16..304d031 100644
> --- a/drivers/staging/rtl8712/rtl8712_recv.c
> +++ b/drivers/staging/rtl8712/rtl8712_recv.c
> @@ -61,13 +61,13 @@ void r8712_init_recv_priv(struct recv_priv *precvpriv,
>   precvbuf->ref_cnt = 0;
>   precvbuf->adapter = padapter;
>   list_add_tail(>list,
> -  &(precvpriv->free_recv_buf_queue.queue));
> +   (>free_recv_buf_queue.queue));

You did this correctly in v2.

-&(precvpriv->free_recv_buf_queue.queue));
+ >free_recv_buf_queue.queue);

>   precvbuf++;
>   }
>   precvpriv->free_recv_buf_queue_cnt = NR_RECVBUFF;
>   tasklet_init(>recv_tasklet,

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: octeon: Remove unneeded variable

2019-10-28 Thread Dan Carpenter
On Mon, Oct 28, 2019 at 09:36:04AM +0100, Julia Lawall wrote:
> 
> 
> On Mon, 28 Oct 2019, Dan Carpenter wrote:
> 
> > On Sat, Oct 26, 2019 at 07:24:53PM -0300, Cristiane Naves wrote:
> > > Remove unneeded variable used to store return value. Issue found by
> > > coccicheck.
> > >
> > > Signed-off-by: Cristiane Naves 
> > > ---
> > >  drivers/staging/octeon/octeon-stubs.h | 4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/staging/octeon/octeon-stubs.h 
> > > b/drivers/staging/octeon/octeon-stubs.h
> > > index b07f5e2..d53bd801 100644
> > > --- a/drivers/staging/octeon/octeon-stubs.h
> > > +++ b/drivers/staging/octeon/octeon-stubs.h
> > > @@ -1387,9 +1387,7 @@ static inline cvmx_pko_status_t 
> > > cvmx_pko_send_packet_finish(uint64_t port,
> > >   uint64_t queue, union cvmx_pko_command_word0 pko_command,
> > >   union cvmx_buf_ptr packet, cvmx_pko_lock_t use_locking)
> > >  {
> > > - cvmx_pko_status_t ret = 0;
> > > -
> > > - return ret;
> > > + return 0;
> >
> > What is the point of this function anyway?
> 
> Given that it is in octeon-stubs.h, it seems that the point is to get the
> code to compile when COMPILE_TEST is set.  There is a real definition in
> arch/mips/include/asm/octeon/cvmx-pko.h

Oh yeah...  Duh.  I saw that it was in stubs but I just assumed that it
was stubs for something which was never implemented or left over code...

My bad.

regards,
dan carpente

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: octeon: Remove unneeded variable

2019-10-28 Thread Dan Carpenter
On Sat, Oct 26, 2019 at 07:24:53PM -0300, Cristiane Naves wrote:
> Remove unneeded variable used to store return value. Issue found by
> coccicheck.
> 
> Signed-off-by: Cristiane Naves 
> ---
>  drivers/staging/octeon/octeon-stubs.h | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/octeon/octeon-stubs.h 
> b/drivers/staging/octeon/octeon-stubs.h
> index b07f5e2..d53bd801 100644
> --- a/drivers/staging/octeon/octeon-stubs.h
> +++ b/drivers/staging/octeon/octeon-stubs.h
> @@ -1387,9 +1387,7 @@ static inline cvmx_pko_status_t 
> cvmx_pko_send_packet_finish(uint64_t port,
>   uint64_t queue, union cvmx_pko_command_word0 pko_command,
>   union cvmx_buf_ptr packet, cvmx_pko_lock_t use_locking)
>  {
> - cvmx_pko_status_t ret = 0;
> -
> - return ret;
> +     return 0;

What is the point of this function anyway?

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [RESEND PATCH 1/2] staging: rtl8712: Fix Alignment of open parenthesis

2019-10-28 Thread Dan Carpenter
On Fri, Oct 25, 2019 at 06:50:25PM -0700, Joe Perches wrote:
> On Fri, 2019-10-25 at 22:09 -0300, Cristiane Naves wrote:
> > Fix alignment should match open parenthesis.Issue found by checkpatch.
> 
> Beyond doing style cleanups, please always try
> to make the code more readable.
> 
> > diff --git a/drivers/staging/rtl8712/rtl8712_recv.c 
> > b/drivers/staging/rtl8712/rtl8712_recv.c
> []
> > @@ -61,13 +61,13 @@ void r8712_init_recv_priv(struct recv_priv *precvpriv,
> > precvbuf->ref_cnt = 0;
> > precvbuf->adapter = padapter;
> > list_add_tail(>list,
> > -&(precvpriv->free_recv_buf_queue.queue));
> > + &(precvpriv->free_recv_buf_queue.queue));
> 
> Please remove the unnecessary parentheses too
> 

Removing the parentheses increases your chance of the patch being
rejected on the one thing per patch rule...

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [RESEND PATCH] staging: gasket: Fix lines ending with a '('

2019-10-28 Thread Dan Carpenter
When I see a RESEND in the subject, that means you are tell us we messed
up and accidentally ignored your patch.  So then we have to figure out
what went wrong with the process and so we don't mess up again.

It would help us if you put a note under the --- cut off like "I sent
this a month ago and never received a response.  Here is a link to the
email archive so I know that it made it to the list."

I recently had an issue like this where I complained that my patch
wasn't applied and the maintainer said "Oh.  That's odd.  I have it
written down in patchword that I emailed you to ask you do fix the bug
in a different way."  So these sorts of mistakes happen.

regards,
dan carpenter


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH 1/3] Staging: qlge: Rename prefix of a function to qlge

2019-10-25 Thread Dan Carpenter
On Fri, Oct 25, 2019 at 05:28:42PM +0300, Samuil Ivanov wrote:
> Dan you are correct it is a bit of a hodge podge :)
> I think that it is better to have a bigger patches that will rename
> more functions, but I don't this it is good to have all the
> functions renamed in one patch.
> 
> Just in the header file I counted around 55 function definitions,
> and in the source files there are some more.
> So that will make one huge patch.
> 

I don't really have a problem if you rename everything at once.  Or if
you want to rename all of them in a 55 patch patchset that's also fine
with me...

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH 1/3] Staging: qlge: Rename prefix of a function to qlge

2019-10-25 Thread Dan Carpenter
On Fri, Oct 25, 2019 at 12:29:39AM +0300, Samuil Ivanov wrote:
> diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
> index 6ec7e3ce3863..e9f1363c5bf2 100644
> --- a/drivers/staging/qlge/qlge.h
> +++ b/drivers/staging/qlge/qlge.h
> @@ -2262,7 +2262,7 @@ int ql_write_mpi_reg(struct ql_adapter *qdev, u32 reg, 
> u32 data);
>  int ql_unpause_mpi_risc(struct ql_adapter *qdev);
>  int ql_pause_mpi_risc(struct ql_adapter *qdev);
>  int ql_hard_reset_mpi_risc(struct ql_adapter *qdev);
> -int ql_soft_reset_mpi_risc(struct ql_adapter *qdev);
> +int qlge_soft_reset_mpi_risc(struct ql_adapter *qdev);

The patch series doesn't change all the functions so now it's hodge
podge.

>  int ql_dump_risc_ram_area(struct ql_adapter *qdev, void *buf, u32 ram_addr,
> int word_count);
>  int ql_core_dump(struct ql_adapter *qdev, struct ql_mpi_coredump 
> *mpi_coredump);
> diff --git a/drivers/staging/qlge/qlge_dbg.c b/drivers/staging/qlge/qlge_dbg.c
> index 019b7e6a1b7a..df5344e113ca 100644
> --- a/drivers/staging/qlge/qlge_dbg.c
> +++ b/drivers/staging/qlge/qlge_dbg.c
> @@ -1312,7 +1312,7 @@ void ql_get_dump(struct ql_adapter *qdev, void *buff)
>  
>   if (!test_bit(QL_FRC_COREDUMP, >flags)) {
>   if (!ql_core_dump(qdev, buff))
> - ql_soft_reset_mpi_risc(qdev);
> + qlge_soft_reset_mpi_risc(qdev);
>   else
>   netif_err(qdev, drv, qdev->ndev, "coredump failed!\n");
>   } else {
> diff --git a/drivers/staging/qlge/qlge_mpi.c b/drivers/staging/qlge/qlge_mpi.c
> index 9e4226ab..efe893935929 100644
> --- a/drivers/staging/qlge/qlge_mpi.c
> +++ b/drivers/staging/qlge/qlge_mpi.c
> @@ -88,9 +88,10 @@ int ql_write_mpi_reg(struct ql_adapter *qdev, u32 reg, u32 
> data)
>   return status;
>  }
>  
> -int ql_soft_reset_mpi_risc(struct ql_adapter *qdev)
> +int qlge_soft_reset_mpi_risc(struct ql_adapter *qdev)
>  {
>   int status;
> +
>   status = ql_write_mpi_reg(qdev, 0x1010, 1);

This white space change is unrelated.

>   return status;
>  }

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH 04/15] staging: exfat: Clean up return codes - FFS_PERMISSIONERR

2019-10-25 Thread Dan Carpenter
On Thu, Oct 24, 2019 at 12:27:47PM -0400, Valdis Klētnieks wrote:
> On Thu, 24 Oct 2019 09:23:33 -0700, Joe Perches said:
> > On Thu, 2019-10-24 at 11:53 -0400, Valdis Kletnieks wrote:
> 
> > >   if (err) {
> > > - if (err == FFS_PERMISSIONERR)
> > > + if (err == -EPERM)
> > >   err = -EPERM;
> > >   else if (err == FFS_INVALIDPATH)
> > >   err = -EINVAL;
> >
> > These test and assign to same value blocks look kinda silly.
> 
> One patch, one thing.  Those are getting cleaned up in a subsequent patch.:)

I was just giving an impromptu lecture on this last week  The one
thing per patch means we cleanup the fallout that results from the
change.  So if you rename a function then you have re-indent the
parameters etc.  If you remove a cast from (type)(foo + bar) then
remove all the extra parentheses and so on.

(I don't have strong feelings about this patch, but I have just been
trying to explain the one thing rule recently).

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v1 1/5] staging: wfx: fix warnings of no space is necessary

2019-10-22 Thread Dan Carpenter
On Sun, Oct 20, 2019 at 12:36:50PM -0700, Joe Perches wrote:
> > It's sort of tricky to know what "one thing per patch means".
> 
> It seems somewhat arbitrary and based on Greg's understanding
> of the experience of the patch submitter and also the language
> of the potential commit message.

Of course the language of the commit message matters.  You have to
"sell" your commit and convince us why we should apply it.  That's a
life lesson right there...  :P

regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v1 1/5] staging: wfx: fix warnings of no space is necessary

2019-10-20 Thread Dan Carpenter
On Sat, Oct 19, 2019 at 01:02:31PM -0700, Joe Perches wrote:
> diff -u -p a/rtl8723bs/core/rtw_mlme_ext.c b/rtl8723bs/core/rtw_mlme_ext.c
> --- a/rtl8723bs/core/rtw_mlme_ext.c
> +++ b/rtl8723bs/core/rtw_mlme_ext.c
> @@ -1132,7 +1132,7 @@ unsigned int OnAuthClient(struct adapter
>   goto authclnt_fail;
>   }
>  
> - memcpy((void *)(pmlmeinfo->chg_txt), (void *)(p + 2), 
> len);
> + memcpy((void *)(pmlmeinfo->chg_txt), (p + 2), len);

I wonder why it didn't remove the first void cast?

[ The rest of the email is bonus comments for outreachy developers ].

And someone needs to check the final patch probably to remove the extra
parentheses around "(p + 2)".  Those were necessary when for the cast
but not required after the cast is gone.

>   pmlmeinfo->auth_seq = 3;
>   issue_auth(padapter, NULL, 0);
>   set_link_timer(pmlmeext, REAUTH_TO);

It's sort of tricky to know what "one thing per patch means".

-   memset((void *)(&(pHTInfo->SelfHTCap)), 0,
+   memset((&(pHTInfo->SelfHTCap)), 0,
sizeof(pHTInfo->SelfHTCap));

Here the parentheses were never related to the cast so we should leave
them as is.  In other words, in the first example, if we didn't remove
the cast that would be "half a thing per patch" and in the second
example that would be "two things in one patch".

regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v1 1/5] staging: wfx: fix warnings of no space is necessary

2019-10-19 Thread Dan Carpenter
On Sat, Oct 19, 2019 at 04:09:03PM +0100, Jules Irenge wrote:
> Checkpatch was complaining about  space between type cast and the 
> variable. I just get rid of the space. Well I don't know whether this was 
> false positive one.
> 
> Thanks for the feedback. I will update the patch.

No no.  The patch is fine.  I was saying that in the *future* you might
want to look at the void casts.  Some of them are not required and
others are buggy code.

regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v1 1/5] staging: wfx: fix warnings of no space is necessary

2019-10-19 Thread Dan Carpenter
On Sat, Oct 19, 2019 at 03:07:15PM +0100, Jules Irenge wrote:
> diff --git a/drivers/staging/wfx/bh.c b/drivers/staging/wfx/bh.c
> index 3355183fc86c..573216b08042 100644
> --- a/drivers/staging/wfx/bh.c
> +++ b/drivers/staging/wfx/bh.c
> @@ -69,13 +69,13 @@ static int rx_helper(struct wfx_dev *wdev, size_t 
> read_len, int *is_cnf)
>   if (wfx_data_read(wdev, skb->data, alloc_len))
>   goto err;
>  
> - piggyback = le16_to_cpup((u16 *) (skb->data + alloc_len - 2));
> + piggyback = le16_to_cpup((u16 *)(skb->data + alloc_len - 2));
>   _trace_piggyback(piggyback, false);
>  
> - hif = (struct hif_msg *) skb->data;
> + hif = (struct hif_msg *)skb->data;
>   WARN(hif->encrypted & 0x1, "unsupported encryption type");
>   if (hif->encrypted == 0x2) {
> - if (wfx_sl_decode(wdev, (void *) hif)) {
> + if (wfx_sl_decode(wdev, (void *)hif)) {

In the future you may want to go through and remove the (void *) casts.
It's not required here.

> diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
> index f65f7d75e731..effd07957753 100644
> --- a/drivers/staging/wfx/bus_spi.c
> +++ b/drivers/staging/wfx/bus_spi.c
> @@ -90,7 +90,7 @@ static int wfx_spi_copy_to_io(void *priv, unsigned int addr,
>   struct wfx_spi_priv *bus = priv;
>   u16 regaddr = (addr << 12) | (count / 2);
>   // FIXME: use a bounce buffer
> - u16 *src16 = (void *) src;
> + u16 *src16 = (void *)src;

Here we are just getting rid of the constness.  Apparently we are doing
that so we can modify it without GCC pointing out the bug!!  I don't
know the code but this seems very wrong.

>   int ret, i;
>   struct spi_message  m;
>   struct spi_transfer t_addr = {

regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: rtl8723bs: reduce stack usage of rtw_cfg80211_unlink_bss

2019-10-18 Thread Dan Carpenter
On Fri, Oct 18, 2019 at 11:48:37AM +0100, Sudip Mukherjee wrote:
> The build of xtensa allmodconfig gives warning of:
> In function 'rtw_cfg80211_unlink_bss':
> warning: the frame size of 1136 bytes is larger than 1024 bytes
> 
> Instead of having 'select_network' structure as a variable use it as a
> pointer.
> 
> Signed-off-by: Sudip Mukherjee 

Heh.  Nice.

Reviewed-by: Dan Carpenter 

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: rtl8192e: initializing the wep buffer

2019-10-18 Thread Dan Carpenter
On Thu, Oct 17, 2019 at 11:57:58PM -0500, Kangjie Lu wrote:
> The "wep" buffer is not initialized. To avoid memory disclosures,
> the fix initializes it, as peer functions like rtllib_ccmp_set_key
> do.
> 
> Signed-off-by: Kangjie Lu 
> ---
>  drivers/staging/rtl8192e/rtllib_crypt_wep.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/staging/rtl8192e/rtllib_crypt_wep.c 
> b/drivers/staging/rtl8192e/rtllib_crypt_wep.c
> index b1ea650036d2..0931777ed157 100644
> --- a/drivers/staging/rtl8192e/rtllib_crypt_wep.c
> +++ b/drivers/staging/rtl8192e/rtllib_crypt_wep.c
> @@ -232,6 +232,7 @@ static int prism2_wep_set_key(void *key, int len, u8 
> *seq, void *priv)
>   if (len < 0 || len > WEP_KEY_LEN)
>   return -1;
>  
> + memset(wep, 0, sizeof(*wep));
>   memcpy(wep->key, key, len);
>   wep->key_len = len;

If we read beyond wep->key_len then it's probably a bug, right?  It
doesn't matter whether it's zeroed out or not.  Fortunately we never
do:

memcpy(key, wep->key, wep->key_len);

So this change isn't required.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2] staging:rtl8723bs: removed unwanted if..else condition

2019-10-17 Thread Dan Carpenter
On Thu, Oct 17, 2019 at 07:48:26PM +0530, Aliasgar Surti wrote:
> From: Aliasgar Surti 
> 
> There is use of if..elseif..else condition which has same logic
> in all three blocks.
> Removed if..else block and placed log message instead that.
> 
> Signed-off-by: Aliasgar Surti 
> ---
> v2: Fixed alignment problem.

Thanks!

Reviewed-by: Dan Carpenter 

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


[PATCH] staging: wilc1000: potential corruption in wilc_parse_join_bss_param()

2019-10-17 Thread Dan Carpenter
The "rates_len" value needs to be capped so that the memcpy() doesn't
copy beyond the end of the array.

Fixes: c5c77ba18ea6 ("staging: wilc1000: Add SDIO/SPI 802.11 driver")
Signed-off-by: Dan Carpenter 
---
 drivers/staging/wilc1000/wilc_hif.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/wilc1000/wilc_hif.c 
b/drivers/staging/wilc1000/wilc_hif.c
index 0ac2b6ac50b0..e0a95c5cc0d5 100644
--- a/drivers/staging/wilc1000/wilc_hif.c
+++ b/drivers/staging/wilc1000/wilc_hif.c
@@ -479,6 +479,8 @@ void *wilc_parse_join_bss_param(struct cfg80211_bss *bss,
rates_ie = cfg80211_find_ie(WLAN_EID_SUPP_RATES, ies->data, ies->len);
if (rates_ie) {
rates_len = rates_ie[1];
+   if (rates_len > WILC_MAX_RATES_SUPPORTED)
+   rates_len = WILC_MAX_RATES_SUPPORTED;
param->supp_rates[0] = rates_len;
memcpy(>supp_rates[1], rates_ie + 2, rates_len);
}
-- 
2.20.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: iio: ad9834: add a check for devm_clk_get

2019-10-16 Thread Dan Carpenter
On Wed, Oct 16, 2019 at 10:25:40PM +0800, Chuhong Yuan wrote:
> ad9834_probe misses a check for devm_clk_get and may cause problems.
> Add a check like what ad9832 does to fix it.
> 
> Signed-off-by: Chuhong Yuan 

Looks good.

Reviewed-by: Dan Carpenter 

regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging:rtl8723bs: removed unwanted if..else condition

2019-10-16 Thread Dan Carpenter
On Wed, Oct 16, 2019 at 07:37:53PM +0530, Aliasgar Surti wrote:
> --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> @@ -507,19 +507,9 @@ int rtw_cmd_thread(void *context)
>  
>   cmd_process_time = jiffies_to_msecs(jiffies - cmd_start_time);
>   if (cmd_process_time > 1000) {
> - if (pcmd->cmdcode == GEN_CMD_CODE(_Set_Drv_Extra)) {
> - DBG_871X(ADPT_FMT" cmd =%d process_time =%lu > 
> 1 sec\n",
> - ADPT_ARG(pcmd->padapter), 
> pcmd->cmdcode, cmd_process_time);
> - /* rtw_warn_on(1); */
> - } else if (pcmd->cmdcode == 
> GEN_CMD_CODE(_Set_MLME_EVT)) {
> - DBG_871X(ADPT_FMT" cmd =%d, process_time =%lu > 
> 1 sec\n",
> - ADPT_ARG(pcmd->padapter), 
> pcmd->cmdcode, cmd_process_time);
> - /* rtw_warn_on(1); */
> - } else {
> - DBG_871X(ADPT_FMT" cmd =%d, process_time =%lu > 
> 1 sec\n",
> - ADPT_ARG(pcmd->padapter), 
> pcmd->cmdcode, cmd_process_time);
> - /* rtw_warn_on(1); */
> - }
> + DBG_871X(ADPT_FMT "cmd= %d process_time= %lu > 1 sec\n",
> +  ADPT_ARG(pcmd->padapter), pcmd->cmdcode,
> +   cmd_process_time);

This last line is aligned to the wrong parenthesis.  It should be:

DBG_871X(ADPT_FMT "cmd= %d process_time= %lu > 1 sec\n",
 ADPT_ARG(pcmd->padapter), pcmd->cmdcode,
 cmd_process_time);

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH 2/4] staging: KPC2000: kpc2000_spi.c: Fix style issues (alignment)

2019-10-16 Thread Dan Carpenter
On Wed, Oct 16, 2019 at 12:49:25AM -0700, Chandra Annamaneni wrote:
> Resolved: "CHECK: Alignment should match open parenthesis" from checkpatch
> 

I think you accidentally copied the wrong commit message.  This one
and the next are the same.  This description doesn't match the patch.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH 5/7] PCI/PM: Make power management op coding style consistent

2019-10-16 Thread Dan Carpenter
On Mon, Oct 14, 2019 at 06:00:14PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas 
> 
> Some of the power management ops use this style:
> 
>   struct device_driver *drv = dev->driver;
>   if (drv && drv->pm && drv->pm->prepare(dev))
> drv->pm->prepare(dev);
> 
> while others use this:
> 
>   const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;

I like this patch a lot, especially the direct returns.  But it
occurs to me that in the future this conditional would look better as

const struct dev_pm_ops *pm = driver_to_pm(dev->driver);

or something.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 1/2] dmaengine: avalon: Intel Avalon-MM DMA Interface for PCIe

2019-10-15 Thread Dan Carpenter
Ah that's fine then.  And since wr_flags[hw->d2h_last_id] is just
true/false then it doesn't matter if it races.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 1/2] dmaengine: avalon: Intel Avalon-MM DMA Interface for PCIe

2019-10-15 Thread Dan Carpenter
On Tue, Oct 15, 2019 at 01:24:50PM +0200, Alexander Gordeev wrote:
> On Thu, Oct 10, 2019 at 02:30:34PM +0300, Dan Carpenter wrote:
> > On Thu, Oct 10, 2019 at 10:51:45AM +0200, Alexander Gordeev wrote:
> > > On Wed, Oct 09, 2019 at 09:53:23PM +0300, Dan Carpenter wrote:
> > > > > > > + u32 *rd_flags = hw->dma_desc_table_rd.cpu_addr->flags;
> > > > > > > + u32 *wr_flags = hw->dma_desc_table_wr.cpu_addr->flags;
> > > > > > > + struct avalon_dma_desc *desc;
> > > > > > > + struct virt_dma_desc *vdesc;
> > > > > > > + bool rd_done;
> > > > > > > + bool wr_done;
> > > > > > > +
> > > > > > > + spin_lock(lock);
> 
> [*]
> 
> > > > > > > +
> > > > > > > + rd_done = (hw->h2d_last_id < 0);
> > > > > > > + wr_done = (hw->d2h_last_id < 0);
> > > > > > > +
> > > > > > > + if (rd_done && wr_done) {
> > > > > > > + spin_unlock(lock);
> > > > > > > + return IRQ_NONE;
> > > > > > > + }
> > > > > > > +
> > > > > > > + do {
> > > > > > > + if (!rd_done && rd_flags[hw->h2d_last_id])
> > > > > > > + rd_done = true;
> > > > > > > +
> > > > > > > + if (!wr_done && wr_flags[hw->d2h_last_id])
> > > > > > > + wr_done = true;
> > > > > > > + } while (!rd_done || !wr_done);


Thinking about this some more, my initial instinct was still correct
actually.  If we're holding the lock to prevent the CPU from writing
to it then how is hw->d2h_last_id updated in the other thread?  Either
it must deadlock or it must be a race condition.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH -next] staging: rtl8723bs: remove unnecessary null check

2019-10-15 Thread Dan Carpenter
On Tue, Oct 15, 2019 at 07:40:53PM +0800, YueHaibing wrote:
> Null check before kfree is redundant, so remove it.
> This is detected by coccinelle.
> 
> Signed-off-by: YueHaibing 
> ---
>  drivers/staging/rtl8723bs/core/rtw_xmit.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_xmit.c 
> b/drivers/staging/rtl8723bs/core/rtw_xmit.c
> index 7011c2a..4597f4f 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_xmit.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_xmit.c
> @@ -2210,8 +2210,7 @@ void rtw_free_hwxmits(struct adapter *padapter)
>   struct xmit_priv *pxmitpriv = >xmitpriv;
>  
>   hwxmits = pxmitpriv->hwxmits;
> - if (hwxmits)
> - kfree(hwxmits);
> + kfree(hwxmits);

Just do:

kfree(pxmitpriv->hwxmits);

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 1/2] dmaengine: avalon: Intel Avalon-MM DMA Interface for PCIe

2019-10-15 Thread Dan Carpenter
On Tue, Oct 15, 2019 at 01:24:50PM +0200, Alexander Gordeev wrote:
> On Thu, Oct 10, 2019 at 02:30:34PM +0300, Dan Carpenter wrote:
> > On Thu, Oct 10, 2019 at 10:51:45AM +0200, Alexander Gordeev wrote:
> > > On Wed, Oct 09, 2019 at 09:53:23PM +0300, Dan Carpenter wrote:
> > > > > > > + u32 *rd_flags = hw->dma_desc_table_rd.cpu_addr->flags;
> > > > > > > + u32 *wr_flags = hw->dma_desc_table_wr.cpu_addr->flags;
> > > > > > > + struct avalon_dma_desc *desc;
> > > > > > > + struct virt_dma_desc *vdesc;
> > > > > > > + bool rd_done;
> > > > > > > + bool wr_done;
> > > > > > > +
> > > > > > > + spin_lock(lock);
> 
> [*]

[ snip ]

> I struggle to realize how the spinlock I use (see [*] above) does not
> protect the reader.

Argh  I'm really sorry.  I completely didn't see the spinlock.  :P

I am embarrassed.  Wow...

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP bridge driver

2019-10-15 Thread Dan Carpenter
[  These are automated text from the 0-day bot -dan ]

Hi Xin,

Thank you for the patch! Perhaps something to improve:

url:
https://github.com/0day-ci/linux/commits/Xin-Ji/dt-bindings-drm-bridge-anx7625-MIPI-to-DP-transmitter-binding/20191014-043019

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot 
Reported-by: Dan Carpenter 

New smatch warnings:
drivers/gpu/drm/bridge/analogix/anx7625.c:1274 anx7625_start_dp_work() error: 
uninitialized symbol 'sp_tx_lane_count'.

# 
https://github.com/0day-ci/linux/commit/152a82b6747f10d6c13c7a422173947c2f2e1aa2
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 152a82b6747f10d6c13c7a422173947c2f2e1aa2
vim +/sp_tx_lane_count +1274 drivers/gpu/drm/bridge/analogix/anx7625.c

152a82b6747f10 Xin Ji 2019-10-11  1253  static void 
anx7625_start_dp_work(struct anx7625_data *ctx)
152a82b6747f10 Xin Ji 2019-10-11  1254  {
152a82b6747f10 Xin Ji 2019-10-11  1255  int ret;
152a82b6747f10 Xin Ji 2019-10-11  1256  u8 buf[MAX_DPCD_BUFFER_SIZE];
152a82b6747f10 Xin Ji 2019-10-11  1257  u8 hdcp_cap;
152a82b6747f10 Xin Ji 2019-10-11  1258  struct device *dev = 
>client->dev;
152a82b6747f10 Xin Ji 2019-10-11  1259  u8 sp_tx_bw; /* linktraining 
banwidth */
152a82b6747f10 Xin Ji 2019-10-11  1260  u8 sp_tx_lane_count; /* link 
training lane count */
152a82b6747f10 Xin Ji 2019-10-11  1261  
152a82b6747f10 Xin Ji 2019-10-11  1262  if (ctx->hpd_high_cnt >= 2) {
152a82b6747f10 Xin Ji 2019-10-11  1263  
DRM_DEV_DEBUG_DRIVER(dev, "anx7625 filter useless HPD\n");
152a82b6747f10 Xin Ji 2019-10-11  1264  return;
152a82b6747f10 Xin Ji 2019-10-11  1265  }
152a82b6747f10 Xin Ji 2019-10-11  1266  
152a82b6747f10 Xin Ji 2019-10-11  1267  ctx->hpd_high_cnt++;
152a82b6747f10 Xin Ji 2019-10-11  1268  
152a82b6747f10 Xin Ji 2019-10-11  1269  sp_tx_get_rx_bw(ctx, _tx_bw);
152a82b6747f10 Xin Ji 2019-10-11  1270  
152a82b6747f10 Xin Ji 2019-10-11  1271  sp_tx_aux_dpcdread_bytes(ctx, 
0x00, 0x00, DPCD_MAX_LANE_COUNT,
152a82b6747f10 Xin Ji 2019-10-11  1272   1, 
_tx_lane_count);

^
Smatch thinks sp_tx_aux_dpcdread_bytes() can fail and we don't check
for errors.

152a82b6747f10 Xin Ji 2019-10-11  1273  
152a82b6747f10 Xin Ji 2019-10-11 @1274  sp_tx_lane_count = 
sp_tx_lane_count & 0x1f;
152a82b6747f10 Xin Ji 2019-10-11  1275  sp_tx_aux_dpcdread_bytes(ctx, 
0x06, 0x80, 0x28, 1, buf);/* read Bcap */
152a82b6747f10 Xin Ji 2019-10-11  1276  
152a82b6747f10 Xin Ji 2019-10-11  1277  hdcp_cap = buf[0] & 0x01;
152a82b6747f10 Xin Ji 2019-10-11  1278  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: wfx: fix potential vulnerability to spectre

2019-10-11 Thread Dan Carpenter
On Fri, Oct 11, 2019 at 12:35:36PM +, Jerome Pouiller wrote:
> On Friday 11 October 2019 14:10:35 CEST Greg Kroah-Hartman wrote:
> > On Fri, Oct 11, 2019 at 10:15:54AM +, Jerome Pouiller wrote:
> > > From: Jérôme Pouiller 
> > >
> > > array_index_nospec() should be applied after a bound check.
> > >
> > > Fixes: 9bca45f3d6924f19f29c0d019e961af3f41bdc9e ("staging: wfx: allow to 
> > > send 802.11 frames")
> > 
> > No need for the full sha1, this should be:
> > Fixes: 9bca45f3d692 ("staging: wfx: allow to send 802.11 frames")
> 
> I copy-pasted information from kbuild robot notification.
> 
> I suggest that commit-id in robot notification is also cut down to 12
> characters. Or even better, to use this snippet:
> 
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot 
> Fixes: 9bca45f3d692 ("staging: wfx: allow to send 802.11 frames")
> 
> (I added l...@lists.01.org in CC but, I am not sure it is the correct
> ML. I am sorry if it is not the case)

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git 
staging-testing
head:   d49d1c76b96ebf39539e93d5ab7943a01ef70e4f
commit: 9bca45f3d6924f19f29c0d019e961af3f41bdc9e [55/111] staging: wfx: allow 
to send 802.11 frames

If you cut and paste then you the "[55/111] " text isn't right either.
Also kbuild works on rebase-able trees as well as non-rebase/published
trees so the hash could change as well.

I am a little bit surprised that checkpatch.pl doesn't complain about
this, though.  You could consider adding that?

regards,
dan carpenter


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: isdn: remove assignment in if conditionals

2019-10-11 Thread Dan Carpenter
This ISDN stuff is going to be deleted soon.  Just leave it as is.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [Outreachy kernel] [PATCH 1/5] staging: octeon: remove typedef declaration for cvmx_wqe_t

2019-10-11 Thread Dan Carpenter
On Fri, Oct 11, 2019 at 11:59:52AM +0300, Wambui Karuga wrote:
> On Fri, Oct 11, 2019 at 08:18:25AM +0200, Julia Lawall wrote:
> > 
> > 
> > On Fri, 11 Oct 2019, Wambui Karuga wrote:
> > 
> > > Remove typedef declaration from struct cvmx_wqe_t in
> > 
> > You can remove the _t from the name as well.
> Should I remove the _t from all the enums/structs?

The _t means typedef (sort of, generally).

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v3 1/4] staging: rtl8723bs: Remove comparisons to NULL in conditionals

2019-10-11 Thread Dan Carpenter
On Thu, Oct 10, 2019 at 04:15:29PM +0300, Wambui Karuga wrote:
>   psetauthparm = rtw_zmalloc(sizeof(struct setauth_parm));
> - if (psetauthparm == NULL) {
> - kfree(pcmd);
> + if (!psetauthparm) {
> + kfree((unsigned char *)pcmd);

This new cast is unnecessary and weird.

>   res = _FAIL;
>   goto exit;
>   }

regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 0/2] Add initial support for slimport anx7625

2019-10-11 Thread Dan Carpenter
On Fri, Oct 11, 2019 at 02:20:47AM +, Xin Ji wrote:
> Hi all,
> 
> The following series add initial support for the Slimport ANX7625 
> transmitter, a
> ultra-low power Full-HD 4K MIPI to DP transmitter designed for portable 
> device.
> 
> This is the initial version, any mistakes, please let me know, I will fix it 
> in
> the next series.
> 
> Thanks,
> Xin
> 

I'm not a domain expert but I like these patches now.

Reviewed-by: Dan Carpenter 

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] KPC2000: kpc2000_spi.c: Fix style issues (line length)

2019-10-11 Thread Dan Carpenter
On Thu, Oct 10, 2019 at 02:54:59PM +, Matt Sickler wrote:
> > static struct mtd_partition p2kr0_spi1_parts[] = {
> >-   { .name = "SLOT_4", .size = 7798784,.offset = 0, 
> >   },
> >-   { .name = "SLOT_5", .size = 7798784,.offset = 
> >MTDPART_OFS_NXTBLK},
> >-   { .name = "SLOT_6", .size = 7798784,.offset = 
> >MTDPART_OFS_NXTBLK},
> >-   { .name = "SLOT_7", .size = 7798784,.offset = 
> >MTDPART_OFS_NXTBLK},
> >-   { .name = "CS1_EXTRA",  .size = MTDPART_SIZ_FULL,   .offset = 
> >MTDPART_OFS_NXTBLK},
> >+   { .name = "SLOT_4",  .size = 7798784,  .offset = 0,},
> >+   { .name = "SLOT_5",  .size = 7798784,  .offset = MTDPART_OFS_NXTBLK},
> >+   { .name = "SLOT_6",  .size = 7798784,  .offset = MTDPART_OFS_NXTBLK},
> >+   { .name = "SLOT_7",  .size = 7798784,  .offset = MTDPART_OFS_NXTBLK},
> >+   { .name = "CS1_EXTRA",  .size = MTDPART_SIZ_FULL, .offset = 
> >MTDPART_OFS_NXTBLK},
> > };
> >
> > static struct flash_platform_data p2kr0_spi0_pdata = {
> 
> Is the line length limit a hard rule or can exceptions be made?  I
> really feel that these data tables are more easily read when they're
> formatted like tables...

Exceptions can be made.  It's probably not worth it though because
you have to be really aggressive about shooting down patches.  Ask
yourself if there aren't more important battles to fight when human
lifespans are so short?  I already rejected one change for you.  To me
the new table looks okay, though.

regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 1/2] dmaengine: avalon: Intel Avalon-MM DMA Interface for PCIe

2019-10-10 Thread Dan Carpenter
On Thu, Oct 10, 2019 at 10:51:45AM +0200, Alexander Gordeev wrote:
> On Wed, Oct 09, 2019 at 09:53:23PM +0300, Dan Carpenter wrote:
> > > > > + u32 *rd_flags = hw->dma_desc_table_rd.cpu_addr->flags;
> > > > > + u32 *wr_flags = hw->dma_desc_table_wr.cpu_addr->flags;
> > > > > + struct avalon_dma_desc *desc;
> > > > > + struct virt_dma_desc *vdesc;
> > > > > + bool rd_done;
> > > > > + bool wr_done;
> > > > > +
> > > > > + spin_lock(lock);
> > > > > +
> > > > > + rd_done = (hw->h2d_last_id < 0);
> > > > > + wr_done = (hw->d2h_last_id < 0);
> > > > > +
> > > > > + if (rd_done && wr_done) {
> > > > > + spin_unlock(lock);
> > > > > + return IRQ_NONE;
> > > > > + }
> > > > > +
> > > > > + do {
> > > > > + if (!rd_done && rd_flags[hw->h2d_last_id])
> > > > > + rd_done = true;
> > > > > +
> > > > > + if (!wr_done && wr_flags[hw->d2h_last_id])
> > > > > + wr_done = true;
> > > > > + } while (!rd_done || !wr_done);
> > > > 
> > > > This loop is very strange.  It feels like the last_id indexes needs
> > > > to atomic or protected from racing somehow so we don't do an out of
> > > > bounds read.
> 
> [...]
> 
> > You're missing my point.  When we set
> > hw->d2h_last_id = 1;
> [1]
> > ...
> > hw->d2h_last_id = 2;
> [2]
> 
> > There is a tiny moment where ->d2h_last_id is transitioning from 1 to 2
> > where its value is unknown.  We're in a busy loop here so we have a
> > decent chance of hitting that 1/1000,000th of a second.  If we happen to
> > hit it at exactly the right time then we're reading from a random
> > address and it will cause an oops.
> > 
> > We have to use atomic_t types or something to handle race conditions.
> 
> Err.. I am still missing the point :( In your example I do see a chance
> for a reader to read out 1 at point in time [2] - because of SMP race.
> But what could it be other than 1 or 2?
> 

The 1 to 2 transition was a poorly chosen example, but a -1 to 1
trasition is better.  The cpu could write a byte at a time.  So maybe
it only wrote the two highest bytes so now it's 0x.  It's not -1 and
it's not 1 and it's not a valid index.

> Anyways, all code paths dealing with h2d_last_id and d2h_last_id indexes
> are protected with a spinlock.

You have to protect both the writer and the reader.  (That's why this
bug is so easy to spot).  https://lwn.net/Articles/793253/

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP bridge driver

2019-10-10 Thread Dan Carpenter
On Thu, Oct 10, 2019 at 12:53:15PM +0300, Dan Carpenter wrote:
> This code is *so* much nicer than before.  I hope you feel good about
> the changes.  It makes me happy to look at this code now.
> 
> On Thu, Oct 10, 2019 at 09:34:19AM +, Xin Ji wrote:
> > +static int edid_read(struct anx7625_data *ctx,
> > +u8 offset, u8 *pblock_buf)
> > +{
> > +   int ret, cnt;
> > +   struct device *dev = >client->dev;
> > +
> > +   for (cnt = 0; cnt < EDID_TRY_CNT; cnt++) {
  ^

> > +   sp_tx_aux_wr(ctx, offset);
> > +   /* set I2C read com 0x01 mot = 0 and read 16 bytes */
> > +   ret = sp_tx_aux_rd(ctx, 0xf1);
> > +
> > +   if (ret) {
> > +   sp_tx_rst_aux(ctx);
> > +   DRM_DEV_DEBUG_DRIVER(dev, "edid read failed, reset!\n");
> > +   cnt++;
^

I mean that it's incremented twice, yeah?

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: sm750fb: Potential uninitialized field in "pll"

2019-10-10 Thread Dan Carpenter
On Wed, Oct 09, 2019 at 09:38:08PM -0700, Yizhuo wrote:
> Inside function set_chip_clock(), struct pll is supposed to be
> initialized in sm750_calc_pll_value(), if condition
> "diff < mini_diff" in sm750_calc_pll_value() cannot be fulfilled,
> then some field of pll will not be initialized but used in
> function sm750_format_pll_reg(), which is potentially unsafe.
> 
> Signed-off-by: Yizhuo 

The patch is correct, but it doesn't apply to linux-next any more.  Can
you re-write it on top of the most recent staging-next and resend?

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP bridge driver

2019-10-10 Thread Dan Carpenter
This code is *so* much nicer than before.  I hope you feel good about
the changes.  It makes me happy to look at this code now.

On Thu, Oct 10, 2019 at 09:34:19AM +, Xin Ji wrote:
> +static int edid_read(struct anx7625_data *ctx,
> +  u8 offset, u8 *pblock_buf)
> +{
> + int ret, cnt;
> + struct device *dev = >client->dev;
> +
> + for (cnt = 0; cnt < EDID_TRY_CNT; cnt++) {
> + sp_tx_aux_wr(ctx, offset);
> + /* set I2C read com 0x01 mot = 0 and read 16 bytes */
> + ret = sp_tx_aux_rd(ctx, 0xf1);
> +
> + if (ret) {
> + sp_tx_rst_aux(ctx);
> + DRM_DEV_DEBUG_DRIVER(dev, "edid read failed, reset!\n");
> + cnt++;

I don't think you should increment cnt.  It's just a counter.

> + } else {
> + ret = anx7625_reg_block_read(ctx, ctx->i2c.rx_p0_client,
> +  AP_AUX_BUFF_START,
> +  MAX_DPCD_BUFFER_SIZE,
> +  pblock_buf);
> + if (!ret)
> + break;
> + }
> + }
> +
> + if (cnt == EDID_TRY_CNT)

And it could mean that "cnt > EDID_TRY_CNT".

> + return -EIO;
> +
> + return 0;
> +}
> +
> +static int segments_edid_read(struct anx7625_data *ctx,
> +   u8 segment, u8 *buf, u8 offset)
> +{
> + u8 cnt;
> + int ret;
> + struct device *dev = >client->dev;
> +
> + /* write address only */
> + ret = anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> + AP_AUX_ADDR_7_0, 0x30);
> + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> +  AP_AUX_COMMAND, 0x04);
> + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> +  AP_AUX_CTRL_STATUS,
> +  AP_AUX_CTRL_ADDRONLY | AP_AUX_CTRL_OP_EN);
> +
> + ret |= wait_aux_op_finish(ctx);
> + /* write segment address */
> + ret |= sp_tx_aux_wr(ctx, segment);
> + /* data read */
> + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> +  AP_AUX_ADDR_7_0, 0x50);
> + if (ret) {
> + DRM_ERROR("IO error : aux initial failed.\n");
> + return ret;
> + }
> +
> + for (cnt = 0; cnt < EDID_TRY_CNT; cnt++) {
> + sp_tx_aux_wr(ctx, offset);
> + /* set I2C read com 0x01 mot = 0 and read 16 bytes */
> + ret = sp_tx_aux_rd(ctx, 0xf1);
> +
> + if (ret) {
> + ret = sp_tx_rst_aux(ctx);
> + DRM_DEV_ERROR(dev, "segment read failed, reset!\n");
> + cnt++;

Same.

> + } else {
> + ret = anx7625_reg_block_read(ctx, ctx->i2c.rx_p0_client,
> +  AP_AUX_BUFF_START,
> +      MAX_DPCD_BUFFER_SIZE, buf);
> + if (!ret)
> + break;
> + }
> + }
> +
> + if (cnt == EDID_TRY_CNT)
> + return -EIO;
> +
> + return 0;
> +}

regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [Outreachy kernel] [PATCH] staging: qlge: Fix multiple assignments warning by splitting the assignement into two each

2019-10-10 Thread Dan Carpenter
I was just about to give a newbie a Reviewed-by cookie until I saw it
was a Joe Perches patch without a commit message or a sign off.  And
then I was annoyed that I had invested any time in it at all.  I even
dropped out of my email client for this!

:P

If you want to resend as a proper commit then you can still have my
Reviewed-by I guess.

Reviewed-by: Dan Carpenter 

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 0/4] staging: rtl8723bs: Style clean-up in rtw_mlme.c

2019-10-10 Thread Dan Carpenter
On Thu, Oct 10, 2019 at 07:49:04AM +0300, Wambui Karuga wrote:
> This patchset addresses multiple style and formatting issues reported by
> checkpatch.pl in drivers/staging/rtl8723bs/core/rtw_mlme.c
> 
> PATCH v2 of the series corrects the "patchest" mispelling in the
> original cover letter and provides a clearer subject line.

The cover letter isn't stored in the git log so spelling is not
important.  The original subjects were fine as well.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH 3/4] staging: rtl8723bs: Remove comparisons to booleans in conditionals.

2019-10-10 Thread Dan Carpenter
On Thu, Oct 10, 2019 at 06:39:23AM +0300, Wambui Karuga wrote:
>   if (is_primary_adapter(adapter))
>   DBG_871X("IsBtDisabled =%d, IsBtControlLps =%d\n", 
> hal_btcoex_IsBtDisabled(adapter), hal_btcoex_IsBtControlLps(adapter));
>  
> - if ((adapter_to_pwrctl(adapter)->bFwCurrentInPSMode == true)
> - && (hal_btcoex_IsBtControlLps(adapter) == false)
> + if ((adapter_to_pwrctl(adapter)->bFwCurrentInPSMode)
> + && !(hal_btcoex_IsBtControlLps(adapter))

Delete the extra parentheses as well, because they don't make sense when
we remove the == false comparison.  It's part of doing "one thing" is to
the whole thing and not leave bits undone.

&& !hal_btcoex_IsBtControlLps(adapter)

>   ) {
>   u8 bEnterPS;
>  
> @@ -2047,7 +2047,7 @@ static int rtw_check_join_candidate(struct mlme_priv 
> *mlme
>  
>  
>   /* check bssid, if needed */
> - if (mlme->assoc_by_bssid == true) {
> + if (mlme->assoc_by_bssid) {
>   if (memcmp(competitor->network.MacAddress, mlme->assoc_bssid, 
> ETH_ALEN))
>   goto exit;
>   }
> @@ -2805,7 +2805,6 @@ void rtw_update_ht_cap(struct adapter *padapter, u8 
> *pie, uint ie_len, u8 channe
>   struct mlme_ext_info *pmlmeinfo = &(pmlmeext->mlmext_info);
>   u8 cbw40_enable = 0;
>  
> -
>   if (!phtpriv->ht_option)
>   return;
>  
> @@ -2815,7 +2814,7 @@ void rtw_update_ht_cap(struct adapter *padapter, u8 
> *pie, uint ie_len, u8 channe
>   DBG_871X("+rtw_update_ht_cap()\n");
>  
>   /* maybe needs check if ap supports rx ampdu. */
> - if ((phtpriv->ampdu_enable == false) && (pregistrypriv->ampdu_enable == 
> 1)) {
> + if (!(phtpriv->ampdu_enable) && pregistrypriv->ampdu_enable == 1) {

Same.

>   if (pregistrypriv->wifi_spec == 1) {
>   /* remove this part because testbed AP should disable 
> RX AMPDU */
>   /* phtpriv->ampdu_enable = false; */

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: qlge: Fix multiple assignments warning by splitting the assignement into two each

2019-10-10 Thread Dan Carpenter
On Wed, Oct 09, 2019 at 09:10:29PM +0100, Jules Irenge wrote:
> Fix multiple assignments warning " check
>  issued by checkpatch.pl tool:
> "CHECK: multiple assignments should be avoided".
> 
> Signed-off-by: Jules Irenge 
> ---
>  drivers/staging/qlge/qlge_dbg.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/qlge/qlge_dbg.c b/drivers/staging/qlge/qlge_dbg.c
> index 086f067fd899..69bd4710c5ec 100644
> --- a/drivers/staging/qlge/qlge_dbg.c
> +++ b/drivers/staging/qlge/qlge_dbg.c
> @@ -141,8 +141,10 @@ static int ql_get_serdes_regs(struct ql_adapter *qdev,
>   u32 *direct_ptr, temp;
>   u32 *indirect_ptr;
>  
> - xfi_direct_valid = xfi_indirect_valid = 0;
> - xaui_direct_valid = xaui_indirect_valid = 1;
> + xfi_indirect_valid = 0;
> + xfi_direct_valid = xfi_indirect_valid;
> + xaui_indirect_valid = 1;
> + xaui_direct_valid = xaui_indirect_valid

The original code is fine here.  Just ignore checkpatch on this.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH 0/7] Fix various compilation issues with wfx driver

2019-10-09 Thread Dan Carpenter
On Wed, Oct 09, 2019 at 03:13:14PM +, Jerome Pouiller wrote:
> On Tuesday 8 October 2019 17:10:56 CEST Greg Kroah-Hartman wrote:
> > On Tue, Oct 08, 2019 at 09:42:47AM +, Jerome Pouiller wrote:
> > > From: Jérôme Pouiller 
> > >
> > > Most of problems are related to big-endian architectures.
> > 
> > kbuild still reports 2 errors with these patches applied:
> > 
> > Regressions in current branch:
> > 
> > drivers/staging/wfx/hif_tx.c:82:2-8: preceding lock on line 65
> 
> As I replied to Julia, this behavior is intended.
> 
> > drivers/staging/wfx/main.c:188:14-21: ERROR: PTR_ERR applied after 
> > initialization to constant on line 183
> 
> This is a false positive, as confirmed by Dan.
> 
> You may also notice:
> 
>   drivers/staging/wfx/scan.c:207 wfx_scan_work() warn: inconsistent returns 
> 'sem:>scan.lock'
> 
> I also consider it as a false positive.

Yeah.  I thought it might be.  The beauty of 0day bot is that normally
the warnings come really quick after the original author wrote the code
so it's fresh in their heads.  I suspected it might be a false positive
but I wasn't sure either way and I try not to spend a lot of time
reviewing those warnings.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 1/2] dmaengine: avalon: Intel Avalon-MM DMA Interface for PCIe

2019-10-09 Thread Dan Carpenter
On Wed, Oct 09, 2019 at 04:58:12PM +0200, Alexander Gordeev wrote:
> On Wed, Oct 09, 2019 at 03:14:41PM +0300, Dan Carpenter wrote:
> > > +config AVALON_DMA_PCI_VENDOR_ID
> > > + hex "PCI vendor ID"
> > > + default "0x1172"
> > > +
> > > +config AVALON_DMA_PCI_DEVICE_ID
> > > + hex "PCI device ID"
> > > + default "0xe003"
> > 
> > This feels wrong.  Why isn't it known in advance.
> 
> Because device designers would likely use they own IDs. The ones I
> put are just defaults inherited from the (Altera) reference design.
> 
> > > + u32 *rd_flags = hw->dma_desc_table_rd.cpu_addr->flags;
> > > + u32 *wr_flags = hw->dma_desc_table_wr.cpu_addr->flags;
> > > + struct avalon_dma_desc *desc;
> > > + struct virt_dma_desc *vdesc;
> > > + bool rd_done;
> > > + bool wr_done;
> > > +
> > > + spin_lock(lock);
> > > +
> > > + rd_done = (hw->h2d_last_id < 0);
> > > + wr_done = (hw->d2h_last_id < 0);
> > > +
> > > + if (rd_done && wr_done) {
> > > + spin_unlock(lock);
> > > + return IRQ_NONE;
> > > + }
> > > +
> > > + do {
> > > + if (!rd_done && rd_flags[hw->h2d_last_id])
> > > + rd_done = true;
> > > +
> > > + if (!wr_done && wr_flags[hw->d2h_last_id])
> > > + wr_done = true;
> > > + } while (!rd_done || !wr_done);
> > 
> > This loop is very strange.  It feels like the last_id indexes needs
> > to atomic or protected from racing somehow so we don't do an out of
> > bounds read.
> 
> My bad. I should have put a comment on this. This polling comes from my
> reading of the Intel documentation:
> 
> "The MSI interrupt notifies the host when a DMA operation has completed.
> After the host receives this interrupt, it can poll the DMA read or write
> status table to determine which entry or entries have the done bit set."
> 
> "The Descriptor Controller writes a 1 to the done bit of the status DWORD
> to indicate successful completion. The Descriptor Controller also sends
> an MSI interrupt for the final descriptor. After receiving this MSI,
> host software can poll the done bit to determine status."
> 
> I sense an ambiguity above. It sounds possible an MSI interrupt could be
> delivered before corresponding done bit is set. May be imperfect wording..
> Anyway, the loop does look weird and in reality I doubt I observed the
> done bit unset even once. So I put this polling just in case.

You're missing my point.  When we set
hw->d2h_last_id = 1;
...
hw->d2h_last_id = 2;

There is a tiny moment where ->d2h_last_id is transitioning from 1 to 2
where its value is unknown.  We're in a busy loop here so we have a
decent chance of hitting that 1/1000,000th of a second.  If we happen to
hit it at exactly the right time then we're reading from a random
address and it will cause an oops.

We have to use atomic_t types or something to handle race conditions.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [staging:staging-testing 41/59] drivers/staging/wfx/main.c:47:14-21: ERROR: PTR_ERR applied after initialization to constant on line 42

2019-10-09 Thread Dan Carpenter
On Wed, Oct 09, 2019 at 02:21:47PM +, Jerome Pouiller wrote:
> On Wednesday 9 October 2019 08:51:10 CEST Rong Chen wrote:
> > On 10/7/19 4:36 PM, Jerome Pouiller wrote:
> > > On Friday 4 October 2019 12:48:32 CEST kbuild test robot wrote:
> > > [...]
> > >>>> drivers/staging/wfx/main.c:47:14-21: ERROR: PTR_ERR applied after 
> > >>>> initialization to constant on line 42
> > >> vim +47 drivers/staging/wfx/main.c
> > >>
> > >>  30
> > >>  31  struct gpio_desc *wfx_get_gpio(struct device *dev, int 
> > >> override, const char *label)
> > >>  32  {
> > >>  33  struct gpio_desc *ret;
> > >>  34  char label_buf[256];
> > >>  35
> > >>  36  if (override >= 0) {
> > >>  37  snprintf(label_buf, sizeof(label_buf), 
> > >> "wfx_%s", label);
> > >>  38  ret = ERR_PTR(devm_gpio_request_one(dev, 
> > >> override, GPIOF_OUT_INIT_LOW, label_buf));
> > >>  39  if (!ret)
> > >>  40  ret = gpio_to_desc(override);
> > >>  41  } else if (override == -1) {
> > >>> 42  ret = NULL;
> > >>  43  } else {
> > >>  44  ret = devm_gpiod_get(dev, label, GPIOD_OUT_LOW);
> > >>  45  }
> > >>  46  if (IS_ERR(ret) || !ret) {
> > >>> 47  if (!ret || PTR_ERR(ret) == -ENOENT)
> > >>  48  dev_warn(dev, "gpio %s is not 
> > >> defined\n", label);
> > >>  49  else
> > >>  50  dev_warn(dev, "error while requesting 
> > >> gpio %s\n", label);
> > >>  51  ret = NULL;
> > >>  52  } else {
> > >>  53  dev_dbg(dev, "using gpio %d for %s\n", 
> > >> desc_to_gpio(ret), label);
> > >>  54  }
> > >>  55  return ret;
> > >>  56  }
> > >>  57
> > > I think that this report is a false positive or I missed something?
> > >
> > Sorry for the inconvenience, but we confirmed that the error first
> > appeared since commit 0096214a59.
> 
> Hi Rong,
> 
> Err... I continue to not understand the meaning of this warning. If
> override != -1 then ret is not constant, isn't?

It's a false positive.  Those happen.  Just ignore it.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: rtl8723bs: hal: Fix memcpy calls

2019-10-09 Thread Dan Carpenter
On Tue, Oct 01, 2019 at 09:58:55PM +0300, Dan Carpenter wrote:
> On Tue, Oct 01, 2019 at 06:13:21PM +0300, Denis Efremov wrote:
> > Just found an official documentation to this issue:
> > https://gcc.gnu.org/gcc-4.9/porting_to.html
> > "Null pointer checks may be optimized away more aggressively
> > ...
> > The pointers passed to memmove (and similar functions in ) must 
> > be non-null
> > even when nbytes==0, so GCC can use that information to remove the check 
> > after the
> > memmove call. Calling copy(p, NULL, 0) can therefore deference a null 
> > pointer and crash."
> > 
> 
> Correct.  In glibc those functions are annotated as non-NULL.
> 
> extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
>  size_t __n) __THROW __nonnull ((1, 2));

I was wrong on this.  It's built into GCC so it doesn't matter how it's
annotated.

> 
> We aren't going to do that in the kernel.  A second difference is that
> in the kernel we use -fno-delete-null-pointer-checks so it doesn't
> delete the NULL checks.

But it's true that the kernel has -fno-delete-null-pointer-checks so I
don't think this is worth patching.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 0/3] staging: wfx: Make some functions static

2019-10-09 Thread Dan Carpenter
Thanks!

regars,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH RFC v2 2/2] dmaengine: avalon: Intel Avalon-MM DMA Interface for PCIe test

2019-10-09 Thread Dan Carpenter
quot;, __FUNCTION__, __LINE__, ret);
> +
> + return ret;
> +}
> +
> +static int kthread_xfer_rw_sg(struct dma_chan *chan,
> +   enum dma_data_direction dir,
> +   dma_addr_t dev_addr,
> +   struct scatterlist *sg, unsigned int sg_len,
> +   void (*xfer_callback)(void *dma_async_param))
> +{
> + struct xfer_callback_info info;
> + int ret;
> + int i;
> +
> + const int nr_reps = nr_dma_reps;

Do it directly.

> +
> + while (!kthread_should_stop()) {
> + init_callback_info(, nr_reps);
> +
> + for (i = 0; i < nr_reps; i++) {
> + ret = submit_xfer_sg(chan, dir,
> + dev_addr, sg, sg_len,
> + xfer_callback, );
> + if (ret < 0)
> + goto err;
> + }
> +
> + dma_async_issue_pending(chan);
> +
> + ret = wait_for_completion_interruptible();
> + if (ret)
> + goto err;
> + }
> +
> + return 0;
> +
> +err:
> + while (!kthread_should_stop())
> + cond_resched();
> +
> + return ret;
> +}
> +
> +struct kthread_xfer_rw_sg_data {
> + struct dma_chan *chan;
> + enum dma_data_direction dir;
> + dma_addr_t dev_addr;
> + struct scatterlist *sg;
> + unsigned int sg_len;
> + void (*xfer_callback)(void *dma_async_param);
> +};
> +
> +static int __kthread_xfer_rw_sg(void *_data)
> +{
> + struct kthread_xfer_rw_sg_data *data = _data;
> +
> + return kthread_xfer_rw_sg(data->chan, data->dir,
> +   data->dev_addr, data->sg, data->sg_len,
> +   data->xfer_callback);
> +}
> +
> +static int __xfer_rw_sg_smp(struct dma_chan *chan,
> + enum dma_data_direction dir,
> + dma_addr_t dev_addr,
> + struct scatterlist *sg, unsigned int sg_len,
> + void (*xfer_callback)(void *dma_async_param))
> +{
> + struct kthread_xfer_rw_sg_data data = {
> + chan, dir,
> + dev_addr, sg, sg_len,
> + xfer_callback
> + };
> + struct task_struct *task;
> + struct task_struct **tasks;
> + int nr_tasks = dmas_per_cpu * num_online_cpus();
> + int n, cpu;
> + int ret = 0;
> + int i = 0;
> +
> + tasks = kmalloc(sizeof(tasks[0]) * nr_tasks, GFP_KERNEL);

kmalloc_array().

> + if (!tasks)
> + return -ENOMEM;
> +
> + for (n = 0; n < dmas_per_cpu; n++) {
> + for_each_online_cpu(cpu) {
> + if (i >= nr_tasks) {
> + ret = -ENOMEM;
> + goto kthread_err;
> + }
> +
> + task = kthread_create(__kthread_xfer_rw_sg,
> +   , "av-dma-sg-%d-%d", cpu, n);
> + if (IS_ERR(task)) {
> + ret = PTR_ERR(task);
> + goto kthread_err;
> + }
> +
> + kthread_bind(task, cpu);
> +
> + tasks[i] = task;
> + i++;
> + }
> + }
> +
> + for (i = 0; i < nr_tasks; i++)
> + wake_up_process(tasks[i]);
> +
> + /*
> +  * Run child kthreads until user sent a signal (i.e Ctrl+C)
> +  * and clear the signal to avid user program from being killed.
> +  */
> + schedule_timeout_interruptible(MAX_SCHEDULE_TIMEOUT);
> + flush_signals(current);
> +
> +kthread_err:
> + for (i = 0; i < nr_tasks; i++)
> + kthread_stop(tasks[i]);

This will Oops when you try free uninitialized data.

The way to do error handling is:
1) Use good label names which say what the label does.  "goto free_foo;"
2) Keep track of the most recently allocated thing.  "foo"
3) Free most recent thing.  That will free everything and it *won't*
   free things which haven't been allocated.

Here it's complicated because we have to break out of a loop.  The rules
for loops are:

4) free the partial iteration before the goto.
5) free down to zero

for (i = 0; i < cnt; i++) {
foo = alloc();
if (!foo)
goto unwind_loop;
foo->bar = alloc();
if (!bar) {
free(foo);
goto unwind_loop;
}
foo->bar->baz = alloc();
if (!baz) {
free(foo->bar);
free(foo);
goto unwind_loop;
}
array[i] = foo;
}

return 0;

unwind_loop:
while (--i >= 0) {
free(array[i]->bar->baz);
free(array[i]->bar);
free(array[i]);
}
free(array);

return -ENOMEM;

My other comment would be that you're sacrificing a lot of readability
to add debug code to this driver...  It's hard to review.

> +
> + kfree(tasks);
> +
> + return ret;
> +}

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH 1/3] staging: wfx: Make function 'sram_write_dma_safe', 'load_firmware_secure' static

2019-10-09 Thread Dan Carpenter
On Wed, Oct 09, 2019 at 09:03:37PM +0800, zhengbin wrote:
> -int sram_write_dma_safe(struct wfx_dev *wdev, u32 addr, const u8 *buf, 
> size_t len)
> +static int
> +sram_write_dma_safe(struct wfx_dev *wdev, u32 addr, const u8 *buf, size_t 
> len)

Either declaration style is fine, but keep it consistent within the
file.  Here the style should be:

static int sram_write_dma_safe(struct wfx_dev *wdev, u32 addr, const u8 *buf,
   size_t len)

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP bridge driver

2019-10-09 Thread Dan Carpenter
On Wed, Oct 09, 2019 at 02:30:32PM +0300, Dan Carpenter wrote:
> > +   platform = kzalloc(sizeof(*platform), GFP_KERNEL);
> > +   if (!platform) {
> > +   DRM_DEV_ERROR(dev, "failed to allocate driver data\n");
> > +   ret = -ENOMEM;
> > +   goto exit;
> 
> return -ENOMEM;
> 
> > +   }
> > +
> > +   pdata = >pdata;
> > +
> > +   /* device tree parsing function call */
> > +   ret = anx7625_parse_dt(>dev, pdata);
> > +   if (ret != 0)   /* if occurs error */
> > +   goto err0;
> 
> != 0 is double negative.  Choose better label names.  Remove the obvious
> comment.
> 
> if (ret)
>   goto free_platform;
> 
> > +
> > +   anx7625_init_gpio(platform);
> > +
> > +   /* to access global platform data */
> > +   platform->client = client;
> > +   i2c_set_clientdata(client, platform);
> > +
> > +   if (platform->pdata.extcon_supported) {
> > +   /* get extcon device from DTS */
> > +   platform->extcon = extcon_get_edev_by_phandle(>dev, 0);
> > +   if (PTR_ERR(platform->extcon) == -EPROBE_DEFER)
> > +   goto err0;
> 
> Preserve the error code.
> 
> > +   if (IS_ERR(platform->extcon)) {
> > +   DRM_DEV_ERROR(>dev,
> > + "can not get extcon device!");
> > +   goto err0;
> 
> Prerve the error code.
> 
> > +   }
> > +
> > +   ret = anx7625_extcon_notifier_init(platform);
> > +   if (ret < 0)
> > +   goto err0;
> > +   }
> > +
> > +   atomic_set(>power_status, 0);
> > +
> > +   mutex_init(>lock);
> > +
> > +   if (platform->pdata.gpio_intr_hpd) {
> > +   INIT_WORK(>work, anx7625_work_func);
> > +   platform->workqueue = create_workqueue("anx7625_work");
> > +   if (!platform->workqueue) {
> > +   DRM_DEV_ERROR(dev, "failed to create work queue\n");
> > +   ret = -ENOMEM;
> > +   goto err1;
> 
> This goto will crash.  Because you have forgotten what the most recently
> allocated resource was.  It should be "goto free_platform;" still.
> 
> > +   }
> > +
> > +   platform->pdata.hpd_irq =
> > +   gpiod_to_irq(platform->pdata.gpio_intr_hpd);
> > +   if (platform->pdata.hpd_irq < 0) {
> > +   DRM_DEV_ERROR(dev, "failed to get gpio irq\n");
> > +   goto err1;
> 
> goto free_wq;
> 
> > +   }
> > +
> > +   ret = request_threaded_irq(platform->pdata.hpd_irq,
> > +  NULL, anx7625_intr_hpd_isr,
> > +  IRQF_TRIGGER_FALLING |
> > +  IRQF_TRIGGER_RISING |
> > +  IRQF_ONESHOT,
> > +  "anx7625-hpd", platform);
> > +   if (ret < 0) {
> > +   DRM_DEV_ERROR(dev, "failed to request irq\n");
> > +   goto err1;
> > +   }
> > +
> > +   ret = irq_set_irq_wake(platform->pdata.hpd_irq, 1);
> > +   if (ret < 0) {
> > +   DRM_DEV_ERROR(dev, "Request irq for hpd");
> > +   DRM_DEV_ERROR(dev, "interrupt wake set fail\n");
> > +   goto err1;
> > +   }
> > +
> > +   ret = enable_irq_wake(platform->pdata.hpd_irq);
> > +   if (ret < 0) {
> > +   DRM_DEV_ERROR(dev, "Enable irq for HPD");
> > +   DRM_DEV_ERROR(dev, "interrupt wake enable fail\n");
> > +   goto err1;
> > +   }
> > +   }
> > +
> > +   if (anx7625_register_i2c_dummy_clients(platform, client) != 0) {
> 
> Preserve the error code.
> 
>   ret = anx7625_register_i2c_dummy_clients();
>   if (ret)
>   goto free_platform;
> 

I meant goto free_wq here.  That's the most recent allocation.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 1/2] dmaengine: avalon: Intel Avalon-MM DMA Interface for PCIe

2019-10-09 Thread Dan Carpenter
l);
> + desc->src_hi = cpu_to_le32((src >> 32));
> + desc->dst_lo = cpu_to_le32(dest & 0xul);
> + desc->dst_hi = cpu_to_le32((dest >> 32));
> + desc->ctl_dma_len = cpu_to_le32((size >> 2) | (desc_id << 18));
> + desc->reserved[0] = cpu_to_le32(0x0);
> + desc->reserved[1] = cpu_to_le32(0x0);
> + desc->reserved[2] = cpu_to_le32(0x0);
> +}
> +
> +static
> +int setup_descs(struct dma_desc *descs, unsigned int desc_id,
> + enum dma_data_direction direction,
> + dma_addr_t dev_addr, dma_addr_t host_addr, unsigned int len,
> + unsigned int *_set)
> +{
> + int nr_descs = 0;
> + unsigned int set = 0;
> + dma_addr_t src;
> + dma_addr_t dest;
> +
> + if (direction == DMA_TO_DEVICE) {
> + src = host_addr;
> + dest = dev_addr;
> + } else if (direction == DMA_FROM_DEVICE) {
> + src = dev_addr;
> + dest = host_addr;
> + } else {
> + BUG();
> + return -EINVAL;
> + }
> +
> + if (unlikely(desc_id > DMA_DESC_MAX - 1)) {

if (desc_id >= DMA_DESC_MAX) {

> + BUG();
> + return -EINVAL;
> + }
> +
> + if (WARN_ON(!len))
> + return -EINVAL;
> +
> + while (len) {
> + unsigned int xfer_len = min_t(unsigned int, len, 
> AVALON_DMA_MAX_TANSFER_SIZE);
> +
> + setup_desc(descs, desc_id, dest, src, xfer_len);
> +
> + set += xfer_len;
> +
> + nr_descs++;
> + if (nr_descs >= DMA_DESC_MAX)
> + break;
> +
> + desc_id++;
> + if (desc_id >= DMA_DESC_MAX)
> + break;
> +
> + descs++;
> +
> + dest += xfer_len;
> + src += xfer_len;
> +
> + len -= xfer_len;
> + }
> +
> + *_set = set;
> +
> + return nr_descs;
> +}
> +
> +int setup_descs_sg(struct dma_desc *descs, unsigned int desc_id,
> +enum dma_data_direction direction,
> +dma_addr_t dev_addr,
> +struct scatterlist *__sg, unsigned int __sg_len,
> +struct scatterlist *sg_start, unsigned int sg_offset,
> +struct scatterlist **_sg_stop, unsigned int *_sg_set)
> +{
> + struct scatterlist *sg;
> + dma_addr_t sg_addr;
> + unsigned int sg_len;
> + unsigned int sg_set;
> + int nr_descs = 0;
> + int ret;
> + int i;
> +
> + /*
> +  * Find the SGE that the previous xfer has stopped on -
> +  * it should exist.
> +  */
> + for_each_sg(__sg, sg, __sg_len, i) {
> + if (sg == sg_start)
> + break;
> +
> + dev_addr += sg_dma_len(sg);
> + }
> +
> + if (WARN_ON(i >= __sg_len))
> + return -EINVAL;
> +
> + /*
> +  * The offset can not be longer than the SGE length.
> +  */
> + sg_len = sg_dma_len(sg);
> + if (WARN_ON(sg_len < sg_offset))
> + return -EINVAL;
> +
> + /*
> +  * Skip the starting SGE if it has been fully transmitted.
> +  */
> + if (sg_offset == sg_len) {
> + if (WARN_ON(sg_is_last(sg)))
> + return -EINVAL;
> +
> + dev_addr += sg_len;
> + sg_offset = 0;
> +
> + i++;
> + sg = sg_next(sg);
> + }
> +
> + /*
> +  * Setup as many SGEs as the controller is able to transmit.
> +  */
> + BUG_ON(i >= __sg_len);
> + for (; i < __sg_len; i++) {
> + sg_addr = sg_dma_address(sg);
> + sg_len = sg_dma_len(sg);
> +
> + if (sg_offset) {
> + if (unlikely(sg_len <= sg_offset)) {
> + BUG();
> + return -EINVAL;
> + }
> +
> + dev_addr += sg_offset;
> + sg_addr += sg_offset;
> + sg_len -= sg_offset;
> +
> + sg_offset = 0;
> + }
> +
> + ret = setup_descs(descs, desc_id, direction,
> +   dev_addr, sg_addr, sg_len, _set);
> + if (ret < 0)
> + return ret;
> +
> + if (unlikely((desc_id + ret > DMA_DESC_MAX) ||
> +  (nr_descs + ret > DMA_DESC_MAX))) {
> + BUG();
> + return -ENOMEM;
> + }
> +
> + nr_descs += ret;
> + desc_id += ret;
> +
> + if (desc_id >= DMA_DESC_MAX)
> + break;

We already checked for this.

> +
> + if (unlikely(sg_len != sg_set)) {
> + BUG();
> + return -EINVAL;
> + }
> +
> + if (sg_is_last(sg))
> + break;
> +
> + descs += ret;
> + dev_addr += sg_len;
> +
> + sg = sg_next(sg);
> + }
> +

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP bridge driver

2019-10-09 Thread Dan Carpenter
Are you sure you sent the correct patch?  This has many of the same
style issues I mentioned in the previous email.  The error handling
in edid_read() is wrong.  probe() will still crash if allocating the
work queue fails.

On Wed, Oct 09, 2019 at 09:28:02AM +, Xin Ji wrote:
> The ANX7625 is an ultra-low power 4K Mobile HD Transmitter designed
> for portable device. It converts MIPI DSI/DPI to DisplayPort 1.3 4K.
> 
> The ANX7625 can support both USB Type-C PD feature and MIPI DSI/DPI
> to DP feature. This driver only enabled MIPI DSI/DPI to DP feature.
> 
> Signed-off-by: Xin Ji 
> ---
>  drivers/gpu/drm/bridge/Makefile   |2 +-
>  drivers/gpu/drm/bridge/analogix/Kconfig   |6 +
>  drivers/gpu/drm/bridge/analogix/Makefile  |1 +
>  drivers/gpu/drm/bridge/analogix/anx7625.c | 2132 
> +
>  drivers/gpu/drm/bridge/analogix/anx7625.h |  405 ++
>  5 files changed, 2545 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.c
>  create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.h
> 
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 4934fcf..bcd388a 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -12,8 +12,8 @@ obj-$(CONFIG_DRM_SII9234) += sii9234.o
>  obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
>  obj-$(CONFIG_DRM_TOSHIBA_TC358764) += tc358764.o
>  obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
> -obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
>  obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o
>  obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
> +obj-y += analogix/
>  obj-y += synopsys/
> diff --git a/drivers/gpu/drm/bridge/analogix/Kconfig 
> b/drivers/gpu/drm/bridge/analogix/Kconfig
> index e930ff9..b2f127e 100644
> --- a/drivers/gpu/drm/bridge/analogix/Kconfig
> +++ b/drivers/gpu/drm/bridge/analogix/Kconfig
> @@ -2,3 +2,9 @@
>  config DRM_ANALOGIX_DP
>   tristate
>   depends on DRM
> +
> +config ANALOGIX_ANX7625
> + tristate "Analogix MIPI to DP interface support"
> + help
> + ANX7625 is an ultra-low power 4K mobile HD transmitter designed
> + for portable devices. It converts MIPI/DPI to DisplayPort1.3 4K.
> diff --git a/drivers/gpu/drm/bridge/analogix/Makefile 
> b/drivers/gpu/drm/bridge/analogix/Makefile
> index fdbf3fd..8a52867 100644
> --- a/drivers/gpu/drm/bridge/analogix/Makefile
> +++ b/drivers/gpu/drm/bridge/analogix/Makefile
> @@ -1,3 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_ANALOGIX_ANX7625) += anx7625.o
>  analogix_dp-objs := analogix_dp_core.o analogix_dp_reg.o
>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix_dp.o
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c 
> b/drivers/gpu/drm/bridge/analogix/anx7625.c
> new file mode 100644
> index 000..7bb4e17
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -0,0 +1,2132 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright(c) 2016, Analogix Semiconductor. All rights reserved.
> + *
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#include "anx7625.h"
> +
> +/*
> + * there is a sync issue while access I2C register between AP(CPU) and
> + * internal firmware(OCM), to avoid the race condition, AP should access
> + * the reserved slave address before slave address occurs changes.
> + */
> +static int i2c_access_workaround(struct anx7625_data *ctx,
> +  struct i2c_client *client)
> +{
> + u8 offset;
> + struct device *dev = >dev;
> + struct i2c_client *last_client = ctx->last_client;
> + int ret = 0;
> +
> + if (client != last_client) {


Please reverse this condition.

if (client == ctx->last_client)
return 0;

Get rid of the last_client variable.


> + ctx->last_client = client;
> +
> + if (client == ctx->i2c.tcpc_client)
> + offset = RSVD_00_ADDR;
> + else if (client == ctx->i2c.tx_p0_client)
> + offset = RSVD_D1_ADDR;
> + else if (client == ctx->i2c.tx_p1_client)
> + offset = RSVD_60_ADDR;
> + else if (client == ctx->i2c.rx_p0_client)
> + offset = RSVD_39_ADDR;
> + else if (client == ctx->i2c.rx_p1_client)
> + offset = RSVD_7F_ADDR;
> + else
> + offset = RSVD_00_ADDR;
> +
> + ret = i2c_smbus_write_byte_data(client, offset, 0x00);
> + if (ret < 0)
> + DRM_DEV_ERROR(dev,
> +   

[staging:staging-testing 55/111] drivers/staging/wfx/wfx.h:91 wdev_to_wvif() warn: potential spectre issue 'wdev->vif' [r] (local cap)

2019-10-09 Thread Dan Carpenter
tree:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git 
staging-testing
head:   d49d1c76b96ebf39539e93d5ab7943a01ef70e4f
commit: 9bca45f3d6924f19f29c0d019e961af3f41bdc9e [55/111] staging: wfx: allow 
to send 802.11 frames

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot 
Reported-by: Dan Carpenter 

New smatch warnings:
drivers/staging/wfx/wfx.h:91 wdev_to_wvif() warn: potential spectre issue 
'wdev->vif' [r] (local cap)
drivers/staging/wfx/data_tx.c:479 wfx_tx_get_raw_link_id() warn: signedness bug 
returning '(-2)'

# 
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?id=9bca45f3d6924f19f29c0d019e961af3f41bdc9e
git remote add staging 
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
git remote update staging
git checkout 9bca45f3d6924f19f29c0d019e961af3f41bdc9e
vim +91 drivers/staging/wfx/wfx.h

e16e7f0716a6ba Jérôme Pouiller 2019-09-19  80  
f4a71ba8753d94 Jérôme Pouiller 2019-09-19  81  static inline struct wfx_vif 
*wdev_to_wvif(struct wfx_dev *wdev, int vif_id)
f4a71ba8753d94 Jérôme Pouiller 2019-09-19  82  {
f4a71ba8753d94 Jérôme Pouiller 2019-09-19  83   if (vif_id >= 
ARRAY_SIZE(wdev->vif)) {
f4a71ba8753d94 Jérôme Pouiller 2019-09-19  84   dev_dbg(wdev->dev, 
"requesting non-existent vif: %d\n", vif_id);
f4a71ba8753d94 Jérôme Pouiller 2019-09-19  85   return NULL;
f4a71ba8753d94 Jérôme Pouiller 2019-09-19  86   }

vaf_id = array_index_nospec(wdev->vif, ARRAY_SIZE(wdev->vif)); ?

f4a71ba8753d94 Jérôme Pouiller 2019-09-19  87   if (!wdev->vif[vif_id]) {
f4a71ba8753d94 Jérôme Pouiller 2019-09-19  88   dev_dbg(wdev->dev, 
"requesting non-allocated vif: %d\n", vif_id);
f4a71ba8753d94 Jérôme Pouiller 2019-09-19  89   return NULL;
f4a71ba8753d94 Jérôme Pouiller 2019-09-19  90   }
f4a71ba8753d94 Jérôme Pouiller 2019-09-19 @91   return (struct wfx_vif *) 
wdev->vif[vif_id]->drv_priv;
f4a71ba8753d94 Jérôme Pouiller 2019-09-19  92  }

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] KPC2000: kpc2000_spi.c: Fix alignment and style problems.

2019-10-09 Thread Dan Carpenter
On Tue, Oct 08, 2019 at 08:50:39PM -0700, Chandra Annamaneni wrote:
> diff --git a/drivers/staging/kpc2000/kpc2000_spi.c 
> b/drivers/staging/kpc2000/kpc2000_spi.c
> index 3be33c4..a20f2d7 100644
> --- a/drivers/staging/kpc2000/kpc2000_spi.c
> +++ b/drivers/staging/kpc2000/kpc2000_spi.c
> @@ -30,19 +30,27 @@
>  #include "kpc.h"
>  
>  static struct mtd_partition p2kr0_spi0_parts[] = {
> - { .name = "SLOT_0", .size = 7798784,.offset = 0,
> },
> - { .name = "SLOT_1", .size = 7798784,.offset = 
> MTDPART_OFS_NXTBLK},
> - { .name = "SLOT_2", .size = 7798784,.offset = 
> MTDPART_OFS_NXTBLK},
> - { .name = "SLOT_3", .size = 7798784,.offset = 
> MTDPART_OFS_NXTBLK},
> - { .name = "CS0_EXTRA",  .size = MTDPART_SIZ_FULL,   .offset = 
> MTDPART_OFS_NXTBLK},
> + { .name = "SLOT_0", .size = 7798784,.offset = 0,},
> + { .name = "SLOT_1", .size = 7798784,.offset =
> +  MTDPART_OFS_NXTBLK},

This looks worse than the original code...  :(  You could maybe make it
a little bit tighter if you used space characters.

{ .name = "SLOT_0",.size = 7798784,  .offset = 0,   
 },
{ .name = "SLOT_1",.size = 7798784,  .offset = 
MTDPART_OFS_NXTBLK},
{ .name = "SLOT_2",.size = 7798784,  .offset = 
MTDPART_OFS_NXTBLK},
{ .name = "SLOT_3",.size = 7798784,  .offset = 
MTDPART_OFS_NXTBLK},
{ .name = "CS0_EXTRA", .size = MTDPART_SIZ_FULL, .offset = 
MTDPART_OFS_NXTBLK},

It still goes over 80 characters, but that's okay.  Or we could just
leave the original code as is.

[ snip ]

>  static struct flash_platform_data p2kr0_spi1_pdata = {
>   .name = "SPI1",
>   .nr_parts = ARRAY_SIZE(p2kr0_spi1_parts),
> @@ -165,7 +174,7 @@ kp_spi_read_reg(struct kp_spi_controller_state *cs, int 
> idx)
>   u64 val;
>  
>   addr += idx;
> - if ((idx == KP_SPI_REG_CONFIG) && (cs->conf_cache >= 0))
> + if (idx == KP_SPI_REG_CONFIG && cs->conf_cache >= 0)

I like these changes but Greg doesn't.  So don't bother with this one.

>   return cs->conf_cache;
>  
>   val = readq(addr);

The rest of the changes are fine.  Split them into multiple patches and
resend.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v3] staging: vc04_services: Avoid NULL comparison

2019-10-09 Thread Dan Carpenter
On Tue, Oct 08, 2019 at 07:44:15PM -0700, Nachammai Karuppiah wrote:
> Remove NULL comparison. Issue found using checkpatch.pl
> 
> Signed-off-by: Nachammai Karuppiah 
> 
> ---
> 
> Changes in V2
>- Remove all NULL comparisons in vc04_services/interface directory.
> ---
> 
> changes in V3
>- Fixed warnings. Reported-by: kbuild test robot 
> ---
> 
> Signed-off-by: Nachammai Karuppiah 
> ---
>  .../interface/vchiq_arm/vchiq_2835_arm.c   |  4 ++--


Only one --- cut off line is needed.  You have two Signed-of-by lines
as well.  It should just be:

Remove NULL comparison. Issue found using checkpatch.pl

Signed-off-by: Nachammai Karuppiah 
---
Changes in V2
   - Remove all NULL comparisons in vc04_services/interface directory.
changes in V3
   - Fixed warnings. Reported-by: kbuild test robot 

  .../interface/vchiq_arm/vchiq_2835_arm.c   |  4 ++--

But this doesn't affect the final patch, and we can see all the
information so it's fine.  No need to resend.

Reviewed-by: Dan Carpenter 

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: vchiq: don't leak kernel address

2019-10-08 Thread Dan Carpenter
On Tue, Oct 08, 2019 at 04:21:54PM +0200, Matteo Croce wrote:
> On Tue, Oct 8, 2019 at 3:16 PM Dan Carpenter  wrote:
> >
> > The subject doesn't match the patch.  It should just be "remove useless
> > printk".
> >
> > regards,
> > dan carpenter
> >
> 
> Well, it avoids leaking an address by removing an useless printk.
> It seems that GKH already picked the patch in his staging tree, but
> I'm fine with both subjects, really,

The address wasn't leaked because it was already %pK.  The subject
says there is an info leak security problem, when the opposite is true.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: vchiq: don't leak kernel address

2019-10-08 Thread Dan Carpenter
The subject doesn't match the patch.  It should just be "remove useless
printk".

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH 6/7] staging: wfx: drop calls to BUG_ON()

2019-10-08 Thread Dan Carpenter
On Tue, Oct 08, 2019 at 09:43:01AM +, Jerome Pouiller wrote:
> @@ -56,9 +56,9 @@ static uint8_t fill_tkip_pair(struct hif_tkip_pairwise_key 
> *msg,
>  {
>   uint8_t *keybuf = key->key;
>  
> - WARN_ON(key->keylen != sizeof(msg->tkip_key_data)
> -+ sizeof(msg->tx_mic_key)
> -+ sizeof(msg->rx_mic_key));
> + WARN(key->keylen != sizeof(msg->tkip_key_data)
> + + sizeof(msg->tx_mic_key)
> + + sizeof(msg->rx_mic_key), "inconsistent data");

This is not a comment on the patch since the code was like that
originally, but the " +" should go of the first line:

WARN(key->keylen != sizeof(msg->tkip_key_data) +
sizeof(msg->tx_mic_key) +
sizeof(msg->rx_mic_key),
 "inconsistent data");

That doesn't look too good still...  The error message is sort of
rubbish also.  Anyway the operator goes on the first line.

regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH 4/7] staging: wfx: correctly cast data on big-endian targets

2019-10-08 Thread Dan Carpenter
On Tue, Oct 08, 2019 at 09:43:00AM +, Jerome Pouiller wrote:
> From: Jérôme Pouiller 
> 
> When built for a big-endian target, original code caused error:
> 
> include/uapi/linux/swab.h:242:29: note: expected '__u32 * {aka unsigned 
> int *}' but argument is of type 'struct hif_mib_protected_mgmt_policy *'
> 
> Fixes: f95a29d40782 ("staging: wfx: add HIF commands helpers")
> Reported-by: kbuild test robot 
> Reported-by: Stephen Rothwell 
> Signed-off-by: Jérôme Pouiller 
> ---
>  drivers/staging/wfx/hif_tx_mib.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/wfx/hif_tx_mib.h 
> b/drivers/staging/wfx/hif_tx_mib.h
> index 167c5dec009f..4f132348f5fa 100644
> --- a/drivers/staging/wfx/hif_tx_mib.h
> +++ b/drivers/staging/wfx/hif_tx_mib.h
> @@ -145,7 +145,7 @@ static inline int hif_set_mfp(struct wfx_vif *wvif, bool 
> capable, bool required)
>   }
>   if (!required)
>   val.unpmf_allowed = 1;
> - cpu_to_le32s();
> + cpu_to_le32s((uint32_t *) );

Again, this is fine for now, but in the future there shouldn't be a
space after the cast.  It's to mark that it's a high precedence
operation.

cpu_to_le32s((uint32_t *));

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH 1/7] staging: wfx: simplify memory allocation in wfx_update_filtering()

2019-10-08 Thread Dan Carpenter
These patches are good.  I just have a few nits to point out for future
reference.

On Tue, Oct 08, 2019 at 09:42:58AM +, Jerome Pouiller wrote:
>  static inline int hif_set_beacon_filter_table(struct wfx_vif *wvif,
> -   struct hif_mib_bcn_filter_table 
> *ft)
> +   int tbl_len,
> +   struct hif_ie_table_entry *tbl)
>  {
> - size_t buf_len = struct_size(ft, ie_table, ft->num_of_info_elmts);
> + int ret;
> + struct hif_mib_bcn_filter_table *val;
> + int buf_len = struct_size(val, ie_table, tbl_len);

This is fine for now, but since this is networking code, in the future
could you write your declarations in reverse Christmas tree order?

long laskdfjasldfj asldfkj aslkdfj alskdfj askldfj;
mid asdfasdflkajdflasjdf lkasjdf;
short asdfasd;
shortest i;

>  
> - cpu_to_le32s(>num_of_info_elmts);
> - return hif_write_mib(wvif->wdev, wvif->id,
> -  HIF_MIB_ID_BEACON_FILTER_TABLE, ft, buf_len);

[ snip ]

> diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
> index 2855d14a709c..12198b8f3685 100644
> --- a/drivers/staging/wfx/sta.c
> +++ b/drivers/staging/wfx/sta.c
> @@ -217,14 +217,13 @@ void wfx_update_filtering(struct wfx_vif *wvif)
>   bool filter_bssid = wvif->filter_bssid;
>   bool fwd_probe_req = wvif->fwd_probe_req;
>   struct hif_mib_bcn_filter_enable bf_ctrl;
> - struct hif_mib_bcn_filter_table *bf_tbl;
> - struct hif_ie_table_entry ie_tbl[] = {
> + struct hif_ie_table_entry filter_ies[] = {
>   {
>   .ie_id= WLAN_EID_VENDOR_SPECIFIC,
>   .has_changed  = 1,
>   .no_longer= 1,
>   .has_appeared = 1,
> - .oui = { 0x50, 0x6F, 0x9A},
> + .oui  = { 0x50, 0x6F, 0x9A },

Please don't do unrelated white space changes in their own patches.

>   }, {
>   .ie_id= WLAN_EID_HT_OPERATION,
>   .has_changed  = 1,

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 0/6] staging: Remove a bounch set of set but not used variables

2019-10-08 Thread Dan Carpenter
Looks good.  Thanks!

Reviewed-by: Dan Carpenter 

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: wlan-ng: p80211wep.c: use lib/crc32

2019-10-08 Thread Dan Carpenter
On Mon, Oct 07, 2019 at 06:15:23PM +0200, Thomas Meyer wrote:
> Dan Carpenter  writes:
> 
> > On Sun, Oct 06, 2019 at 04:07:45PM +0200, Thomas Meyer wrote:
> >> Use lib/crc32 instead of another implementation.
> >> 
> >> Signed-off-by: Thomas Meyer 
> >
> > I always get annoyed whenever anyone asks if people have tested their
> > patches, but have you tested this?
> 
> no annoynence on my side, but... :-)
> 
> Good question. I tell you what I did and then you tell me if I did test!
> 
> So I did this: I did write a small C program that does contain a small
> byte buffer and the extracted CRC32 logic from the wlan driver.
> The program does calculate the CRC32 sum with the extracted logic and by
> calling crc32_le function. but values are the same.
> 
> But as I don't own the hardware I couldn't do a real test with WEP (as
> far as I understand only WEP on this hardware would be affected.)
> 
> So a better test would be to find someone which actually owns the
> hardware and could test the change.
> 
> so... what do you think?

Could you send your test program?

Anyway your testing sounds reasonable so I'm fine with this change.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: rtl8712: Add comment to lock declaration

2019-10-08 Thread Dan Carpenter
On Mon, Oct 07, 2019 at 09:52:48PM +0100, Jules Irenge wrote:
> Add comment to spinlock declaration to fix warning issued by checkpatch.pl
> "CHECK: spinlock_t definition without comment".
> 
> Signed-off-by: Jules Irenge 
> ---
>  drivers/staging/rtl8712/drv_types.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rtl8712/drv_types.h 
> b/drivers/staging/rtl8712/drv_types.h
> index 0c4325073c63..960d8709aada 100644
> --- a/drivers/staging/rtl8712/drv_types.h
> +++ b/drivers/staging/rtl8712/drv_types.h
> @@ -160,7 +160,7 @@ struct _adapter {
>   int pid; /*process id from UI*/
>   struct work_struct wk_filter_rx_ff0;
>   u8 blnEnableRxFF0Filter;
> - spinlock_t lock_rx_ff0_filter;
> + spinlock_t lock_rx_ff0_filter; /*spinlock to protect interrupt request*/

This spinlock seems to be nonsense.  It's only used one time:

drivers/staging/rtl8712/xmit_linux.c
94  void r8712_SetFilter(struct work_struct *work)
95  {
96  struct _adapter *adapter = container_of(work, struct _adapter,
97  wk_filter_rx_ff0);
98  u8  oldvalue = 0x00, newvalue = 0x00;
99  unsigned long irqL;
   100  
   101  oldvalue = r8712_read8(adapter, 0x117);
   102  newvalue = oldvalue & 0xfe;
   103  r8712_write8(adapter, 0x117, newvalue);
   104  
   105  spin_lock_irqsave(>lock_rx_ff0_filter, irqL);
   106  adapter->blnEnableRxFF0Filter = 1;

It only protects writing to ->blnEnableRxFF0Filter but it doesn't
protect reading so it can't possibly work.

   107  spin_unlock_irqrestore(>lock_rx_ff0_filter, irqL);
   108  do {
   109  msleep(100);
   110  } while (adapter->blnEnableRxFF0Filter == 1);
   111  r8712_write8(adapter, 0x117, oldvalue);
   112  }

Also put a space after /* and before */ so the comment looks like:
/* spinlock to protect interrupt request */

But in this case, the comment isn't correct so please just leave it
as-is until someone can fix the locking.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH 2/6] staging: sm750fb: Remove set but not used variable 'uiActualPixelClk'

2019-10-08 Thread Dan Carpenter
On Tue, Oct 08, 2019 at 09:06:11AM +0300, Dan Carpenter wrote:
> On Tue, Oct 08, 2019 at 01:38:58PM +0800, zhengbin wrote:
> > diff --git a/drivers/staging/sm750fb/ddk750_mode.c 
> > b/drivers/staging/sm750fb/ddk750_mode.c
> > index 9722692..e0230f4 100644
> > --- a/drivers/staging/sm750fb/ddk750_mode.c
> > +++ b/drivers/staging/sm750fb/ddk750_mode.c
> > @@ -209,12 +209,11 @@ static int programModeRegisters(struct mode_parameter 
> > *pModeParam,
> >  int ddk750_setModeTiming(struct mode_parameter *parm, enum clock_type 
> > clock)
> >  {
> > struct pll_value pll;
> > -   unsigned int uiActualPixelClk;
> > 
> > pll.input_freq = DEFAULT_INPUT_CLOCK;
> > pll.clock_type = clock;
> > 
> > -   uiActualPixelClk = sm750_calc_pll_value(parm->pixel_clock, );
> > +   sm750_calc_pll_value(parm->pixel_clock, );
> 
> Get rid of this function as well.

Oops.  I'm wrong.  Sorry for the noise.  Leave the function.

regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH 2/6] staging: sm750fb: Remove set but not used variable 'uiActualPixelClk'

2019-10-08 Thread Dan Carpenter
On Tue, Oct 08, 2019 at 01:38:58PM +0800, zhengbin wrote:
> diff --git a/drivers/staging/sm750fb/ddk750_mode.c 
> b/drivers/staging/sm750fb/ddk750_mode.c
> index 9722692..e0230f4 100644
> --- a/drivers/staging/sm750fb/ddk750_mode.c
> +++ b/drivers/staging/sm750fb/ddk750_mode.c
> @@ -209,12 +209,11 @@ static int programModeRegisters(struct mode_parameter 
> *pModeParam,
>  int ddk750_setModeTiming(struct mode_parameter *parm, enum clock_type clock)
>  {
>   struct pll_value pll;
> - unsigned int uiActualPixelClk;
> 
>   pll.input_freq = DEFAULT_INPUT_CLOCK;
>   pll.clock_type = clock;
> 
> - uiActualPixelClk = sm750_calc_pll_value(parm->pixel_clock, );
> + sm750_calc_pll_value(parm->pixel_clock, );

Get rid of this function as well.

regards,
dan carpenter


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH 1/6] staging: bcm2835-audio: Remove set but not used variable 'status'

2019-10-08 Thread Dan Carpenter
On Tue, Oct 08, 2019 at 01:38:57PM +0800, zhengbin wrote:
> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c 
> b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> index c6f9cf1..8a94c5b 100644
> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> @@ -96,13 +96,12 @@ static void audio_vchi_callback(void *param,
>   struct bcm2835_audio_instance *instance = param;
>   struct vc_audio_msg m;
>   int msg_len;
> - int status;
> 
>   if (reason != VCHI_CALLBACK_MSG_AVAILABLE)
>   return;
> 
> - status = vchi_msg_dequeue(instance->vchi_handle,
> -   , sizeof(m), _len, VCHI_FLAGS_NONE);
> + vchi_msg_dequeue(instance->vchi_handle,
> +  , sizeof(m), _len, VCHI_FLAGS_NONE);

This looks like a bug in the code.  flags is VCHI_FLAGS_NONE so it can
return -1 and we should check for that.

>   if (m.type == VC_AUDIO_MSG_TYPE_RESULT) {

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


[staging:staging-next 50/93] drivers/staging/wfx/debug.c:112 wfx_send_hif_msg_read() warn: maybe return -EFAULT instead of the bytes remaining?

2019-10-07 Thread Dan Carpenter
tree:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git 
staging-next
head:   e772cd8c9c9cd3d08715800aabaf50b771b395d9
commit: 4f8b7fabb15df3658564a98971fc67029be1815d [50/93] staging: wfx: allow to 
send commands to chip

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot 
Reported-by: Dan Carpenter 

smatch warnings:
drivers/staging/wfx/debug.c:112 wfx_send_hif_msg_read() warn: maybe return 
-EFAULT instead of the bytes remaining?

# 
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?id=4f8b7fabb15df3658564a98971fc67029be1815d
git remote add staging 
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
git remote update staging
git checkout 4f8b7fabb15df3658564a98971fc67029be1815d
vim +112 drivers/staging/wfx/debug.c

4f8b7fabb15df3 Jérôme Pouiller 2019-09-19   95  static ssize_t 
wfx_send_hif_msg_read(struct file *file, char __user *user_buf,
4f8b7fabb15df3 Jérôme Pouiller 2019-09-19   96  
 size_t count, loff_t *ppos)
4f8b7fabb15df3 Jérôme Pouiller 2019-09-19   97  {
4f8b7fabb15df3 Jérôme Pouiller 2019-09-19   98  struct dbgfs_hif_msg 
*context = file->private_data;
4f8b7fabb15df3 Jérôme Pouiller 2019-09-19   99  int ret;
4f8b7fabb15df3 Jérôme Pouiller 2019-09-19  100  
4f8b7fabb15df3 Jérôme Pouiller 2019-09-19  101  if (count > 
sizeof(context->reply))
4f8b7fabb15df3 Jérôme Pouiller 2019-09-19  102  return -EINVAL;
4f8b7fabb15df3 Jérôme Pouiller 2019-09-19  103  ret = 
wait_for_completion_interruptible(>complete);
4f8b7fabb15df3 Jérôme Pouiller 2019-09-19  104  if (ret)
4f8b7fabb15df3 Jérôme Pouiller 2019-09-19  105  return ret;
4f8b7fabb15df3 Jérôme Pouiller 2019-09-19  106  if (context->ret < 0)
4f8b7fabb15df3 Jérôme Pouiller 2019-09-19  107  return 
context->ret;
4f8b7fabb15df3 Jérôme Pouiller 2019-09-19  108  // Be carefull, write() 
is waiting for a full message while read()
4f8b7fabb15df3 Jérôme Pouiller 2019-09-19  109  // only return a payload
4f8b7fabb15df3 Jérôme Pouiller 2019-09-19  110  ret = 
copy_to_user(user_buf, context->reply, count);
4f8b7fabb15df3 Jérôme Pouiller 2019-09-19  111  if (ret)
4f8b7fabb15df3 Jérôme Pouiller 2019-09-19 @112  return ret;

Yeah.  It should be:

if (copy_to_user(user_buf, context->reply, count))
return -EFAULT;

4f8b7fabb15df3 Jérôme Pouiller 2019-09-19  113  
4f8b7fabb15df3 Jérôme Pouiller 2019-09-19  114  return count;
4f8b7fabb15df3 Jérôme Pouiller 2019-09-19  115  }

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: rtl8712: align arguments with open parenthesis in file rtl8712_led.c

2019-10-07 Thread Dan Carpenter
On Sun, Oct 06, 2019 at 09:39:02PM -0300, Gabriela Bittencourt wrote:
> Cleans up checks of "Alignment should match open parenthesis"
> 
> Signed-off-by: Gabriela Bittencourt 
> ---
>  drivers/staging/rtl8712/rtl8712_led.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rtl8712/rtl8712_led.c 
> b/drivers/staging/rtl8712/rtl8712_led.c
> index db99129d3169..5901026949f2 100644
> --- a/drivers/staging/rtl8712/rtl8712_led.c
> +++ b/drivers/staging/rtl8712/rtl8712_led.c
> @@ -75,7 +75,7 @@ static void BlinkWorkItemCallback(struct work_struct *work);
>   *   Initialize an LED_871x object.
>   */
>  static void InitLed871x(struct _adapter *padapter, struct LED_871x *pLed,
> -  enum LED_PIN_871x  LedPin)
> + enum LED_PIN_871x   LedPin)
         ^^
Delete these extra spaces.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: wlan-ng: p80211wep.c: use lib/crc32

2019-10-07 Thread Dan Carpenter
On Sun, Oct 06, 2019 at 04:07:45PM +0200, Thomas Meyer wrote:
> Use lib/crc32 instead of another implementation.
> 
> Signed-off-by: Thomas Meyer 

I always get annoyed whenever anyone asks if people have tested their
patches, but have you tested this?  It's hard for me to review it
because I don't have the relevant background and because I'm a little
bit stupid.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH 1/5] staging: rtl8723bs: Remove set but not used variable 'i'

2019-10-07 Thread Dan Carpenter
On Sun, Oct 06, 2019 at 05:09:55PM +0800, zhengbin wrote:
> @@ -689,7 +688,7 @@ static s32 update_attrib(struct adapter *padapter, _pkt 
> *pkt, struct pkt_attrib
>   DBG_COUNTER(padapter->tx_logs.core_tx_upd_attrib);
> 
>   _rtw_open_pktfile(pkt, );
> - i = _rtw_pktfile_read(, (u8 *), ETH_HLEN);
> + (void)_rtw_pktfile_read(, (u8 *), ETH_HLEN);

Don't add this "(void)" here.  There is no need and it looks ugly.

_rtw_pktfile_read(, (u8 *), ETH_HLEN);

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: rtl8712: fix boundary condition for n

2019-10-03 Thread Dan Carpenter
On Wed, Oct 02, 2019 at 10:35:19PM +0530, Rohit Sarkar wrote:
> Now that snprintf is replaced by scnprintf n >= MAX_WPA_IE_LEN doesn't
> make sense as the maximum value n can take is MAX_WPA_IE_LEN.
> 
> Signed-off-by: Rohit Sarkar 
> ---

Thanks!

Reviewed-by: Dan Carpenter 

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: exfat: use bdev_sync function directly where needed

2019-10-03 Thread Dan Carpenter
Or we could apply your other patch which trumps both this patch and the
patch to the TODO.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: exfat: use bdev_sync function directly where needed

2019-10-03 Thread Dan Carpenter
I replied to your other thread and I added Saiyam Doshi to the CC list
there.  Just to be clear this patch is a good cleanup and doesn't affect
runtime at all.

In the other thread, I suggested that we leave fs_sync() as a marker
even though it's dead code.  But looking at it now, I think that it's
not really useful.  Future auditors should look for places which call
fs_set_vol_flags(sb, VOL_CLEAN); instead.  That's exactly the places
which call fs_sync().

regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] drivers/staging/exfat - explain the fs_sync() issue in TODO

2019-10-03 Thread Dan Carpenter
On Wed, Oct 02, 2019 at 03:01:35PM -0400, Valdis Klētnieks wrote:
> We've seen several incorrect patches for fs_sync() calls in the exfat driver.
> Add code to the TODO that explains this isn't just a delete code and refactor,
> but that actual analysis of when the filesystem should be flushed to disk
> needs to be done.
> 

This doesn't help at all because no one can be expected to read it.
Put a comment in the code which says something like:

diff --git a/drivers/staging/exfat/exfat_super.c 
b/drivers/staging/exfat/exfat_super.c
index 229ecabe7a93..c1710d99875e 100644
--- a/drivers/staging/exfat/exfat_super.c
+++ b/drivers/staging/exfat/exfat_super.c
@@ -287,6 +287,13 @@ static DEFINE_SEMAPHORE(z_sem);
 
 static inline void fs_sync(struct super_block *sb, bool do_sync)
 {
+   /*
+* Oct 2019:  Please, do not delete this code or the callers.  This
+* code is obviously bogus and many of the callers are dead code, yes,
+* but it may hold clues as to when syncing is required.  Someone needs
+* to go through and audit it really carefully.
+*
+*/
if (do_sync)
bdev_sync(sb);
 }
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: wlan-ng: fix uninitialized variable

2019-10-03 Thread Dan Carpenter
On Wed, Oct 02, 2019 at 08:41:03PM +0300, Denis Efremov wrote:
> The result variable in prism2_connect() can be used uninitialized on path
> !channel --> ... --> is_wep --> sme->key --> sme->key_idx >= NUM_WEPKEYS.
> This patch initializes result with 0.
> 
> Cc: Greg Kroah-Hartman 
> Cc: sta...@vger.kernel.org
> Signed-off-by: Denis Efremov 
> ---
>  drivers/staging/wlan-ng/cfg80211.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/wlan-ng/cfg80211.c 
> b/drivers/staging/wlan-ng/cfg80211.c
> index eee1998c4b18..d426905e187e 100644
> --- a/drivers/staging/wlan-ng/cfg80211.c
> +++ b/drivers/staging/wlan-ng/cfg80211.c
> @@ -441,7 +441,7 @@ static int prism2_connect(struct wiphy *wiphy, struct 
> net_device *dev,
>   int chan = -1;
>   int is_wep = (sme->crypto.cipher_group == WLAN_CIPHER_SUITE_WEP40) ||
>   (sme->crypto.cipher_group == WLAN_CIPHER_SUITE_WEP104);
> - int result;
> + int result = 0;
>   int err = 0;
>  

I can't see any reason why we should have both "err" and "result".
Maybe in olden times "result" used to save positive error codes instead
of negative error codes but now it's just negatives and zero on success.
There is no reason for the exit label either, we could just return
directly.

So could you redo it and get rid of "result" entirely?  Otherwise it
just causes more bugs like this.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: exfat: use bdev_sync function directly where needed

2019-10-03 Thread Dan Carpenter
On Wed, Oct 02, 2019 at 08:47:03PM +0530, Saiyam Doshi wrote:
> fs_sync() is wrapper to bdev_sync(). When fs_sync is called with
> non-zero argument, bdev_sync gets called.
> 
> Most instances of fs_sync is called with false and very few with
> true. Refactor this and makes direct call to bdev_sync() where
> needed and removes fs_sync definition.
> 
> Signed-off-by: Saiyam Doshi 

Reviewed-by: Dan Carpenter 

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: rtl8712: Replace snprintf with scnprintf

2019-10-02 Thread Dan Carpenter
On Wed, Oct 02, 2019 at 05:12:14PM +0530, Rohit Sarkar wrote:
> On Wed, Oct 02, 2019 at 01:57:22PM +0300, Dan Carpenter wrote:
> > 
> > We could leave it as is or change it to "MAX_WPA_IE_LEN - 1".  But I
> > feel like the default should be to leave it as is unless there is a good
> > reason.
> 
> Makes sense, although greg has already merged this. I guess I will
> remove the redundant check then.

You could remove it, but I feel like it's better to check for
"== MAX_WPA_IE_LEN - 1".  They're effectively the same, but to me it
feels cleaner to be explicit how we're handling truncated data.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: rtl8712: Replace snprintf with scnprintf

2019-10-02 Thread Dan Carpenter
On Wed, Oct 02, 2019 at 10:03:51AM +0530, Rohit Sarkar wrote:
> On Tue, Oct 01, 2019 at 10:00:56PM +0300, Dan Carpenter wrote:
> > 
> > No.  scnprintf() returns the number of characters *not counting the
> > NUL terminator*.  So it can be a maximum of MAX_WPA_IE_LEN - 1.
> > 
> > regards,
> > dan carpenter
> 
> TIL :)
> Would the better approach be to just remove the loop or break when n ==
> MAX_WPA_IE_LEN - 1.

We could leave it as is or change it to "MAX_WPA_IE_LEN - 1".  But I
feel like the default should be to leave it as is unless there is a good
reason.

So from a static analysis perspective we wouldn't complain unless/until
the "n" is re-used outside the loop.  So this a chance to make a smarter
static analyzer.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: rtl8723bs: os_dep: Remove return variables

2019-10-02 Thread Dan Carpenter
Someone already sent a patch to remove these functions.  Generally there
should only be empty functions like this in a .h file.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [RESEND PATCH] staging: rtl8192u: Fix indentation for cleaner code

2019-10-02 Thread Dan Carpenter
On Wed, Oct 02, 2019 at 03:49:04PM +0530, Sumera Priyadarsini wrote:
> On Tue, Sep 24, 2019 at 8:47 PM Greg KH  wrote:
> >
> > On Fri, Sep 13, 2019 at 11:31:01PM +0530, Sumera Priyadarsini wrote:
> > > Fixes indentation for if condition in the file r8190_rtl8256.c for better 
> > > readability as suggested by Dan Carpenter.
> >
> > Please wrap your lines at 72 columns.
> >
> >
> I will keep this in mind. I was under the impression that the line
> length must be 80 columns
> but will make the change immediately. To be able to wrap the lines,
> maybe code implementation
> needs to be changed slightly as there is a lot of nesting present in
> the current code?

The 72 character limit is for commit messages, not code.  Checkpatch.pl
will complain at 75 characters.  It's the same rule as email.

regards,
dan carepnter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: rtl8712: Replace snprintf with scnprintf

2019-10-01 Thread Dan Carpenter
On Tue, Oct 01, 2019 at 11:09:26PM +0530, Rohit Sarkar wrote:
> On Tue, Oct 01, 2019 at 11:45:14AM +0300, Dan Carpenter wrote:
> > > diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c 
> > > b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> > > index b08b9a191a34..ff5edcaba64d 100644
> > > --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> > > +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> > > @@ -142,7 +142,7 @@ static noinline_for_stack char 
> > > *translate_scan_wpa(struct iw_request_info *info,
> > >   memset(buf, 0, MAX_WPA_IE_LEN);
> > >   n = sprintf(buf, "wpa_ie=");
> > >   for (i = 0; i < wpa_len; i++) {
> > > - n += snprintf(buf + n, MAX_WPA_IE_LEN - n,
> > > + n += scnprintf(buf + n, MAX_WPA_IE_LEN - n,
> > >   "%02x", wpa_ie[i]);
> > >   if (n >= MAX_WPA_IE_LEN)
> > ^^^
> > It checks for overflow here.  This check is impossible now and doesn't
> > make sense.  The other loop is similar.
> 
> Good catch! I must have overlooked this.
> "n" cannot be greater than MAX_WPA_IE_LEN but it can be equal to that
> value. We can replace the '>=' with '==' so that we don't loop
> unnecessarily when n has reached it's threshold.

No.  scnprintf() returns the number of characters *not counting the
NUL terminator*.  So it can be a maximum of MAX_WPA_IE_LEN - 1.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: rtl8723bs: hal: Fix memcpy calls

2019-10-01 Thread Dan Carpenter
On Tue, Oct 01, 2019 at 06:13:21PM +0300, Denis Efremov wrote:
> Just found an official documentation to this issue:
> https://gcc.gnu.org/gcc-4.9/porting_to.html
> "Null pointer checks may be optimized away more aggressively
> ...
> The pointers passed to memmove (and similar functions in ) must be 
> non-null
> even when nbytes==0, so GCC can use that information to remove the check 
> after the
> memmove call. Calling copy(p, NULL, 0) can therefore deference a null pointer 
> and crash."
> 

Correct.  In glibc those functions are annotated as non-NULL.

extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
 size_t __n) __THROW __nonnull ((1, 2));

We aren't going to do that in the kernel.  A second difference is that
in the kernel we use -fno-delete-null-pointer-checks so it doesn't
delete the NULL checks.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: rtl8712: Replace snprintf with scnprintf

2019-10-01 Thread Dan Carpenter
On Wed, Sep 11, 2019 at 12:25:03AM +0530, Rohit Sarkar wrote:
> Resending as I made a typo in Larry's email id.
> 

I commented on the earlier email, but this email doesn't apply with
`git am` so it would be difficult for Larry to review it.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: rtl8723bs: hal: Fix memcpy calls

2019-10-01 Thread Dan Carpenter
On Mon, Sep 30, 2019 at 05:25:43PM +0300, Denis Efremov wrote:
> On 9/30/19 4:18 PM, David Laight wrote:
> > From: Denis Efremov
> >> Sent: 30 September 2019 12:02
> >> memcpy() in phy_ConfigBBWithParaFile() and PHY_ConfigRFWithParaFile() is
> >> called with "src == NULL && len == 0". This is an undefined behavior.
> > 
> > I'm pretty certain it is well defined (to do nothing).
> 
> Well, technically you are right. However, UBSAN warns about passing NULL
> to memcpy and from the formal point of view this is an undefined behavior [1].
> There were a discussion [2] about interesting implication of assuming that
> memcpy with 0 size and NULL pointer is fine. This could result in that 
> compiler
> assume that pointer is not NULL.

That's true for glibc memcpy() but not for the kernel memcpy().  In the
kernel there are lots of places which do a zero size memcpy().

The glibc attitude is "the standard allows us to put knives here" so
let's put knives everywhere in the path.  And the GCC attitude is let's
silently remove NULL checks instead of just printing a warning that the
NULL check isn't required...  It could really make someone despondent.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] Staging: fbtft: fix memory leak in fbtft_framebuffer_alloc

2019-10-01 Thread Dan Carpenter
On Sun, Sep 29, 2019 at 10:09:45PM -0500, Navid Emamdoost wrote:
> In fbtft_framebuffer_alloc the error handling path should take care of
> releasing frame buffer after it is allocated via framebuffer_alloc, too.
> Therefore, in two failure cases the goto destination is changed to
> address this issue.
> 
> Fixes: c296d5f9957c ("staging: fbtft: core support")
> Signed-off-by: Navid Emamdoost 

Looks good.

Reviewed-by: Dan Carpenter 

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH 2/3] Staging: exfat: exfat_super.c: fixed multiple coding style issues with camelcase and parentheses

2019-10-01 Thread Dan Carpenter
On Sun, Sep 29, 2019 at 09:52:29AM -0500, Jesse Barton wrote:
> Changed Function Names:
> ffsGetVolInfo -> ffs_get_vol_info
> ffsLookupFile -> ffs_lookup_file
> ffsCreateFile -> ffs_create_file
> ffsReadFile -> ffs_read_file
> ffsWriteFile -> ffs_write_file
> ffsTruncateFile -> ffs_truncate_file
> ffsRemoveFile -> ffs_remove_file
> ffsMoveFile -> ffs_move_file
> ffsSetAttr -> ffs_set_attr
> ffsReadStat -> ffs_read_stat
> ffsWriteStat -> ffs_write_stat
> ffsMapCluster -> ffs_map_cluster
> 
> Removed BUG_ON() and added WARN_ON().
> Removed unnecessary parentheses:
> *(dir_entry->Name) - > *dir_entry->Name

Do these other two things in separate patches.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH 2/3] Staging: exfat: exfat_super.c: fixed multiple coding style issues with camelcase and parentheses

2019-10-01 Thread Dan Carpenter
On Sat, Sep 28, 2019 at 07:22:33PM -0500, Jesse Barton wrote:
> Fixed coding style issues with camelcase on functions and various parentheses 
> that were not needed
> 

Do this as two separate patches.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH 3/3] Staging: exfat: exfat_super.c Fixed coding style issues.

2019-10-01 Thread Dan Carpenter
On Sat, Sep 28, 2019 at 07:21:19PM -0500, Jesse Barton wrote:
> Fixed Coding Style issues

Which ones?

> 
> Signed-off-by: Jesse Barton 
> ---
>  drivers/staging/exfat/exfat_super.c | 29 +
>  1 file changed, 9 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/staging/exfat/exfat_super.c 
> b/drivers/staging/exfat/exfat_super.c
> index 3c7e2b7c2195..b9656ec06144 100644
> --- a/drivers/staging/exfat/exfat_super.c
> +++ b/drivers/staging/exfat/exfat_super.c
> @@ -640,8 +640,7 @@ static int ffs_lookup_file(struct inode *inode, char 
> *path, struct file_id_t *fi
>   return ret;
>  }
>  
> -static int ffs_create_file(struct inode *inode, char *path, u8 mode,
> -  struct file_id_t *fid)
> +static int ffs_create_file(struct inode *inode, char *path, u8 mode, struct 
> file_id_t *fid)

I think now checkpatch will complain that the line is too long?  What we
want here is:

static int ffs_create_file(struct inode *inode, char *path, u8 mode,
   struct file_id_t *fid)

[tab][tab][tab][space][space][space]struct file_id_t *fid)

So they line up.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: rtl8188eu: fix null dereference when kzalloc fails

2019-10-01 Thread Dan Carpenter
On Fri, Sep 27, 2019 at 02:44:15PM -0700, Connor Kuehl wrote:
> If kzalloc() returns NULL, the error path doesn't stop the flow of
> control from entering rtw_hal_read_chip_version() which dereferences the
> null pointer. Fix this by adding a 'goto' to the error path to more
> gracefully handle the issue and avoid proceeding with initialization
> steps that we're no longer prepared to handle.
> 
> Also update the debug message to be more consistent with the other debug
> messages in this function.
> 
> Addresses-Coverity: ("Dereference after null check")
> 
> Signed-off-by: Connor Kuehl 
> ---
>  drivers/staging/rtl8188eu/os_dep/usb_intf.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/rtl8188eu/os_dep/usb_intf.c 
> b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
> index 664d93a7f90d..4fac9dca798e 100644
> --- a/drivers/staging/rtl8188eu/os_dep/usb_intf.c
> +++ b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
> @@ -348,8 +348,10 @@ static struct adapter *rtw_usb_if1_init(struct 
> dvobj_priv *dvobj,
>   }
>  

There is another one earlier in the function as well.

drivers/staging/rtl8188eu/os_dep/usb_intf.c
   336  
   337  pnetdev = rtw_init_netdev(padapter);
   338  if (!pnetdev)
   339  goto free_adapter;
   340  SET_NETDEV_DEV(pnetdev, dvobj_to_dev(dvobj));
   341  padapter = rtw_netdev_priv(pnetdev);
   342  
   343  if (padapter->registrypriv.monitor_enable) {
   344  pmondev = rtl88eu_mon_init();
   345  if (!pmondev)
   346  netdev_warn(pnetdev, "Failed to initialize 
monitor interface");

goto free_adapter.

   347  padapter->pmondev = pmondev;
   348  }
   349  
   350  padapter->HalData = kzalloc(sizeof(struct hal_data_8188e), 
GFP_KERNEL);
   351  if (!padapter->HalData)
   352  DBG_88E("cant not alloc memory for HAL DATA\n");
   353  

>   padapter->HalData = kzalloc(sizeof(struct hal_data_8188e), GFP_KERNEL);
> - if (!padapter->HalData)
> - DBG_88E("cant not alloc memory for HAL DATA\n");
> + if (!padapter->HalData) {
> +     DBG_88E("Failed to allocate memory for HAL data\n");

Remove this debug printk.

> + goto free_adapter;
> + }


regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: rtl8723bs: Variable rf_type in function rtw_cfg80211_init_wiphy() could be uninitialized

2019-10-01 Thread Dan Carpenter
On Fri, Sep 27, 2019 at 05:06:51PM -0700, Yizhuo wrote:
> In function rtw_cfg80211_init_wiphy(), local variable "rf_type" could
> be uninitialized if function rtw_hal_get_hwreg() fails to initialize
> it. However, this value is used in function rtw_cfg80211_init_ht_capab()
> and used to decide the value writing to ht_cap, which is potentially
> unsafe.

I feel like this is from a Smatch warning.  Sure, it looks from reading
the code that rtw_hal_get_hwreg() can fail, but actually it cannot.

The longer explanation is that in these rtl drivers if you see a
function with "_hal_" in it that stands for "Hardware Abstraction Layer".
The HAL layer is nonsense.

drivers/staging/rtl8723bs/hal/hal_intf.c
   139  void rtw_hal_get_hwreg(struct adapter *padapter, u8 variable, u8 *val)
   140  {
   141  if (padapter->HalFunc.GetHwRegHandler)
   142  padapter->HalFunc.GetHwRegHandler(padapter, variable, 
val);
   143  }

It looks as if reading the hardware register is an optional feature but
obviously that's not possibly true.

We can use Smatch to find out which functions implement the function
pointer:
~/smatch/smatch_data/db/smdb.py functions GetHwRegHandler

drivers/staging/rtl8723bs/hal/sdio_halinit.c | (struct 
hal_ops)->GetHwRegHandler | GetHwReg8723BS  | 1

So in this driver the ->GetHwRegHandler pointer always points to the
GetHwReg8723BS() function.  Then we can check what the return states
for that function are:

~/smatch/smatch_data/db/smdb.py return_states GetHwReg8723BS

It prints a lot of information but the relevant line is:

drivers/staging/rtl8723bs/hal/sdio_halinit.c | GetHwReg8723BS | 84 |
   | PARAM_SET |   2 |   *$ | 0-u16max |

Which means that the *val is always set and never uninitialized.  This
is after I have rebuilt my Smatch DB several times.  I rebuild it every
day and it has been a long time since I started from scratch.

So removing the HAL layer would make this code parsable by Smatch and it
would make it more readable for human beings as well.  Another option
would be to just delete the "if (padapter->HalFunc.GetHwRegHandler)"
check which would also silence the false positive.  A third option would
be to add "rtw_hal_get_hwreg 2" to the
~/smatch/smatch_data/kernel.ignore_uninitialized_param file.

regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


  1   2   3   4   5   6   7   8   9   10   >