Re: [PATCH v2 1/2] staging: rtl8192u: Add or remove spaces to fix style issues

[Dan Carpenter]

On Mon, Aug 26, 2019 at 11:39:09PM +0530, Sumera Priyadarsini wrote:
> This patch fixes the file r8190_rtl8256.c to avoid the following
> checkpatch.pl warnings:
>   CHECK: spaces preferred around that '<<' (ctx:VxV)
>   CHECK: spaces preferred around that '-' (ctx:VxV)
>   CHECK: No space is necessary after a cast
> 
> Signed-off-by: Sumera Priyadarsini 
> ---
> Changes since v1:
> - Split the commit into two patches
> ---
>  drivers/staging/rtl8192u/r8190_rtl8256.c | 30 
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/rtl8192u/r8190_rtl8256.c 
> b/drivers/staging/rtl8192u/r8190_rtl8256.c
> index 0bedf88525cd..7c7f8247b27a 100644
> --- a/drivers/staging/rtl8192u/r8190_rtl8256.c
> +++ b/drivers/staging/rtl8192u/r8190_rtl8256.c
> @@ -42,9 +42,9 @@ void phy_set_rf8256_bandwidth(struct net_device *dev, enum 
> ht_channel_width Band
>  
>   switch (Bandwidth) {
>   case HT_CHANNEL_WIDTH_20:
> - if (priv->card_8192_version == VERSION_819XU_A
> - || priv->card_8192_version
> - == VERSION_819XU_B) { /* 8256 D-cut, 
> E-cut, xiong: consider it later! */
> + if (priv->card_8192_version == VERSION_819XU_A 
> ||
> + priv->card_8192_version ==
> + VERSION_819XU_B) { /* 8256 D-cut, 
> E-cut, xiong: consider it later! */

These lines are indented too far and the == should go on the first line
like the || does.

if (priv->card_8192_version == VERSION_819XU_A ||
priv->card_8192_version == VERSION_819XU_B) {
/* 8256 D-cut, E-cut, xiong: consider it later! 
*/

Probably do this in a separate patch.  It's sort of not really related
to the other changes.

regards,
dan carpenter


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

Re: [PATCH] staging: vt6656: Use common error handling code in vnt_alloc_bufs()

[Dan Carpenter]

The original code is fine.

There was no chance we were going to apply the patch so there is no need
for any discussion.  I don't know why Markus sent it when he knows.

regards,
dan carpenter

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

Re: rtl8192u - meaning of TO_DO_LIST?

[Dan Carpenter]

On Thu, Aug 22, 2019 at 02:51:46PM -0700, step...@brennan.io wrote:
> Hi all,
> 
> Similar to Colin's question yesterday about the rtl8192u driver, is
> anybody familiar with the TO_DO_LIST define? As I looked through
> checkpatch warnings I saw a particular concerning pattern in
> drivers/staging/rtl8192u/r8192U.h:2183:
> 
> #ifdef TO_DO_LIST
>   if (Adapter->bInHctTest)
>   /* long statement here */
>   else
> #endif
>   /* unindented statement here */
> 
> It seems this code would break if TO_DO_LIST were defined, and I verified
> that it is the case. I can't find any comment or documentation about what
> the goal of this "to do" item is.
> 
> Does anybody know what this is there for? If not, does it make sense to
> just go ahead and remove the code there?

If the code is dead, just delete it.  That's a very simple rule in
staging.

regards,
dan carpenter

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

Re: [PATCH] Staging: isdn/gigaset : Fix bare unsigned warnings and trailing lines errors

[Dan Carpenter]

Hi Martin,

These drivers are ancient and going to be deleted soon.  We're not
accepting cleanups for them at this point.

On Wed, Aug 21, 2019 at 03:27:39PM +, Martin Tomes wrote:
> There are many bare use of unsigned warnings and trailing statements should 
> be on next line errors from checkpatch.pl script.
> Change the code by adding 'unsigned int'. Move 'break' statement of 'switch' 
> command to next line.

For future reference, this should be split up so each patch fixes
one kind of style issue.  And the commit message lines of text are too
long.  The limit is 75 characters for commit messages (like an email).

regards,
dan carpenter

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

Re: [PATCH] staging: rtl8192u: remove redundant assignment to pointer crypt

[Dan Carpenter]

On Thu, Aug 22, 2019 at 09:46:09AM +0100, Colin King wrote:
> From: Colin Ian King 
> 
> The pointer crypt is being set with a value that is never read,
> the assignment is redundant and hence can be removed.
> 
> Thanks to Dan Carpenter for sanity checking that this was indeed
> redundant.
> 
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King 

Thanks!

Reviewed-by: Dan Carpenter 

regards,
dan carpenter

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

Re: Added Realtek rtl8192u driver to staging - static analysis report.

[Dan Carpenter]

On Wed, Aug 21, 2019 at 07:18:39PM +0100, Colin Ian King wrote:
> Hi,
> 
> Static analysis of linux-next picked up an issue with the following commit:
> 
> commit 8fc8598e61f6f384f3eaf1d9b09500c12af47b37
> Author: Jerry Chuang 
> Date:   Tue Nov 3 07:17:11 2009 -0200
> 
> Staging: Added Realtek rtl8192u driver to staging
> 
> In drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c we have:
> 
> CID 48331 (#1 of 1): Unused value (UNUSED_VALUE) assigned_pointer
> 
> Assigning value from ieee->crypt[ieee->tx_keyidx] to crypt here, but
> that stored value is not used.
> 
> 746crypt = ieee->crypt[ieee->tx_keyidx];
> 747if (encrypt)
> 748beacon_buf->capability |=
> cpu_to_le16(WLAN_CAPABILITY_PRIVACY);

Earlir in the function we have:

   695  crypt = ieee->crypt[ieee->tx_keyidx];
   696  
   697  encrypt = ieee->host_encrypt && crypt && crypt->ops &&
   698  ((0 == strcmp(crypt->ops->name, "WEP") || wpa_ie_len));
   699  /* HT ralated element */

So the "crypt" assignment is dublicate and should definitely be removed.
The "if (encrypt) " check looks correct and it sort of matches what we
do in ieee80211_assoc_resp().

   840  encrypt = crypt && crypt->ops;
   841  
   842  if (encrypt)
   843  assoc->capability |= 
cpu_to_le16(WLAN_CAPABILITY_PRIVACY);
   844  

So let's leave it as-is, just delete the crypt assignment.  If you want,
you can send this patch and I can give you a Reviewed-by tag or if you
want I can send the patch and give you Reported-by credit.

regards,
dan carpenter

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

Re: [RFC PATCH 15/15] drivers/acrn: add the support of offline SOS cpu

[Dan Carpenter]

On Fri, Aug 16, 2019 at 10:25:56AM +0800, Zhao Yakui wrote:
> diff --git a/drivers/staging/acrn/acrn_dev.c b/drivers/staging/acrn/acrn_dev.c
> index 0602125..6868003 100644
> --- a/drivers/staging/acrn/acrn_dev.c
> +++ b/drivers/staging/acrn/acrn_dev.c
> @@ -588,6 +588,41 @@ static const struct file_operations fops = {
>  #define SUPPORT_HV_API_VERSION_MAJOR 1
>  #define SUPPORT_HV_API_VERSION_MINOR 0
>  
> +static ssize_t
> +offline_cpu_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> +#ifdef CONFIG_X86
> + u64 cpu, lapicid;
> +
> + if (kstrtoull(buf, 0, ) < 0)
> + return -EINVAL;

Preserve the error code.

ret = kstrtoull(buf, 0, );
if (ret)
return ret;

> +
> + if (cpu_possible(cpu)) {

You can't pass unchecked cpu values to cpu_possible() or it results in
an out of bounds read if cpu is >= than nr_cpu_ids.

regards,
dan carpenter

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

Re: [RFC PATCH 08/15] drivers/acrn: add VM memory management for ACRN char device

[Dan Carpenter]

On Mon, Aug 19, 2019 at 01:32:54PM +0800, Zhao, Yakui wrote:
> In fact as this driver is mainly used for embedded IOT usage, it doesn't
> handle the complex cleanup when such error is encountered. Instead the clean
> up is handled in free_guest_vm.

A use after free here seems like a potential security problem.  Security
matters for IoT...  :(

regards,
dan carpenter

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

Re: [RFC PATCH 11/15] drivers/acrn: add the support of handling emulated ioreq

[Dan Carpenter]

On Fri, Aug 16, 2019 at 10:25:52AM +0800, Zhao Yakui wrote:
> +int acrn_ioreq_create_client(unsigned short vmid,
> +  ioreq_handler_t handler,
> +  void *client_priv,
> +  char *name)
> +{
> + struct acrn_vm *vm;
> + struct ioreq_client *client;
> + int client_id;
> +
> + might_sleep();
> +
> + vm = find_get_vm(vmid);
> + if (unlikely(!vm || !vm->req_buf)) {
> + pr_err("acrn-ioreq: failed to find vm from vmid %d\n", vmid);
> + put_vm(vm);
> + return -EINVAL;
> + }
> +
> + client_id = alloc_client();
> + if (unlikely(client_id < 0)) {
> + pr_err("acrn-ioreq: vm[%d] failed to alloc ioreq client\n",
> +vmid);
> + put_vm(vm);
> + return -EINVAL;
> + }
> +
> + client = acrn_ioreq_get_client(client_id);
> + if (unlikely(!client)) {
> + pr_err("failed to get the client.\n");
> + put_vm(vm);
> + return -EINVAL;

Do we need to clean up the alloc_client() allocation?

regards,
dan carpenter

> + }
> +
> + if (handler) {
> + client->handler = handler;
> + client->acrn_create_kthread = true;
> + }
> +
> + client->ref_vm = vm;
> + client->vmid = vmid;
> + client->client_priv = client_priv;
> + if (name)
> + strncpy(client->name, name, sizeof(client->name) - 1);
> + rwlock_init(>range_lock);
> + INIT_LIST_HEAD(>range_list);
> + init_waitqueue_head(>wq);
> +
> + /* When it is added to ioreq_client_list, the refcnt is increased */
> + spin_lock_bh(>ioreq_client_lock);
> + list_add(>list, >ioreq_client_list);
> + spin_unlock_bh(>ioreq_client_lock);
> +
> + pr_info("acrn-ioreq: created ioreq client %d\n", client_id);
> +
> + return client_id;
> +}

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

Re: [RFC PATCH 10/15] drivers/acrn: add interrupt injection support

[Dan Carpenter]

On Fri, Aug 16, 2019 at 10:25:51AM +0800, Zhao Yakui wrote:
> + case IC_VM_INTR_MONITOR: {
> + struct page *page;
> +
> + ret = get_user_pages_fast(ioctl_param, 1, 1, );
> + if (unlikely(ret != 1) || !page) {
   
Not required.

> + pr_err("acrn-dev: failed to pin intr hdr buffer!\n");
> + return -ENOMEM;
> + }
> +
> + ret = hcall_vm_intr_monitor(vm->vmid, page_to_phys(page));
> + if (ret < 0) {
> + pr_err("acrn-dev: monitor intr data err=%ld\n", ret);
> + return -EFAULT;
> +     }
> + break;
> + }
> +

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

Re: [RFC PATCH 09/15] drivers/acrn: add passthrough device support

[Dan Carpenter]

On Fri, Aug 16, 2019 at 10:25:50AM +0800, Zhao Yakui wrote:
> + case IC_ASSIGN_PTDEV: {
> + unsigned short bdf;
> +
> + if (copy_from_user(, (void *)ioctl_param,

This casting is ugly and you also need a __user tag for Sparse.  Do
something like "void __user *p = ioctl_param;"

> +sizeof(unsigned short)))
> + return -EFAULT;
> +
> + ret = hcall_assign_ptdev(vm->vmid, bdf);
> + if (ret < 0) {
> + pr_err("acrn: failed to assign ptdev!\n");
> + return -EFAULT;

Preserve the error code.  "return ret;".

> + }
> + break;
> + }
> + case IC_DEASSIGN_PTDEV: {
> + unsigned short bdf;
> +
> + if (copy_from_user(, (void *)ioctl_param,
> +sizeof(unsigned short)))
> + return -EFAULT;
> +
> + ret = hcall_deassign_ptdev(vm->vmid, bdf);
> + if (ret < 0) {
> + pr_err("acrn: failed to deassign ptdev!\n");
> + return -EFAULT;
> + }
> + break;
> + }
> +
> + case IC_SET_PTDEV_INTR_INFO: {
> + struct ic_ptdev_irq ic_pt_irq;
> + struct hc_ptdev_irq *hc_pt_irq;
> +
> + if (copy_from_user(_pt_irq, (void *)ioctl_param,
> +sizeof(ic_pt_irq)))
> + return -EFAULT;
> +
> + hc_pt_irq = kmalloc(sizeof(*hc_pt_irq), GFP_KERNEL);
> + if (!hc_pt_irq)
> + return -ENOMEM;
> +
> + memcpy(hc_pt_irq, _pt_irq, sizeof(*hc_pt_irq));

Use memdup_user().

> +
> + ret = hcall_set_ptdev_intr_info(vm->vmid,
> + virt_to_phys(hc_pt_irq));
> + kfree(hc_pt_irq);
> + if (ret < 0) {
> + pr_err("acrn: failed to set intr info for ptdev!\n");
> + return -EFAULT;
> + }
> +
> + break;
> + }

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

Re: [RFC PATCH 08/15] drivers/acrn: add VM memory management for ACRN char device

[Dan Carpenter]

On Fri, Aug 16, 2019 at 10:25:49AM +0800, Zhao Yakui wrote:
> +int hugepage_map_guest(struct acrn_vm *vm, struct vm_memmap *memmap)
> +{
> + struct page *page = NULL, *regions_buf_pg = NULL;
> + unsigned long len, guest_gpa, vma;
> + struct vm_memory_region *region_array;
> + struct set_regions *regions;
> + int max_size = PAGE_SIZE / sizeof(struct vm_memory_region);
> + int ret;
> +
> + if (!vm || !memmap)
> + return -EINVAL;
> +
> + len = memmap->len;
> + vma = memmap->vma_base;
> + guest_gpa = memmap->gpa;
> +
> + /* prepare set_memory_regions info */
> + regions_buf_pg = alloc_page(GFP_KERNEL);
> + if (!regions_buf_pg)
> + return -ENOMEM;
> +
> + regions = kzalloc(sizeof(*regions), GFP_KERNEL);
> + if (!regions) {
> + __free_page(regions_buf_pg);
> + return -ENOMEM;

It's better to do a goto err_free_regions_buf here.  More comments
below.

> + }
> + regions->mr_num = 0;
> + regions->vmid = vm->vmid;
> + regions->regions_gpa = page_to_phys(regions_buf_pg);
> + region_array = page_to_virt(regions_buf_pg);
> +
> + while (len > 0) {
> + unsigned long vm0_gpa, pagesize;
> +
> + ret = get_user_pages_fast(vma, 1, 1, );
> + if (unlikely(ret != 1) || (!page)) {
> + pr_err("failed to pin huge page!\n");
> + ret = -ENOMEM;
> + goto err;

goto err is a red flag.  It's better if error labels do one specific
named thing like:

err_regions:
kfree(regions);
err_free_regions_buf:
__free_page(regions_buf_pg);

We should unwind in the opposite/mirror order from how things were
allocated.  Then we can remove the if statements in the error handling.

In this situation, say the user triggers an -EFAULT in
get_user_pages_fast() in the second iteration through the loop.  That
means that "page" is the non-NULL page from the previous iteration.  We
have already added it to add_guest_map().  But now we're freeing it
without removing it from the map so probably it leads to a use after
free.

The best way to write the error handling in a loop like this is to
clean up the partial iteration that has succeed (nothing here), and then
unwind all the successful iterations at the bottom of the function.
"goto unwind_loop;"

> + }
> +
> + vm0_gpa = page_to_phys(page);
> + pagesize = PAGE_SIZE << compound_order(page);
> +
> + ret = add_guest_map(vm, vm0_gpa, guest_gpa, pagesize);
> + if (ret < 0) {
> + pr_err("failed to add memseg for huge page!\n");
> +     goto err;

So then here, it would be:

pr_err("failed to add memseg for huge page!\n");
put_page(page);
goto unwind_loop;

regards,
dan carpenter

> + }
> +
> + /* fill each memory region into region_array */
> + region_array[regions->mr_num].type = MR_ADD;
> + region_array[regions->mr_num].gpa = guest_gpa;
> + region_array[regions->mr_num].vm0_gpa = vm0_gpa;
> + region_array[regions->mr_num].size = pagesize;
> + region_array[regions->mr_num].prot =
> + (MEM_TYPE_WB & MEM_TYPE_MASK) |
> + (memmap->prot & MEM_ACCESS_RIGHT_MASK);
> + regions->mr_num++;
> + if (regions->mr_num == max_size) {
> + pr_debug("region buffer full, set & renew regions!\n");
> + ret = set_memory_regions(regions);
> + if (ret < 0) {
> + pr_err("failed to set regions,ret=%d!\n", ret);
> + goto err;
> + }
> + regions->mr_num = 0;
> + }
> +
> + len -= pagesize;
> + vma += pagesize;
> + guest_gpa += pagesize;
> + }
> +
> + ret = set_memory_regions(regions);
> + if (ret < 0) {
> + pr_err("failed to set regions, ret=%d!\n", ret);
> + goto err;
> + }
> +
> + __free_page(regions_buf_pg);
> + kfree(regions);
> +
> + return 0;
> +err:
> + if (regions_buf_pg)
> + __free_page(regions_buf_pg);
> + if (page)
> + put_page(page);
> + kfree(regions);
> + return ret;
> +}
> +

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

Re: [RFC PATCH 04/15] drivers/acrn: add the basic framework of acrn char device driver

[Dan Carpenter]

On Fri, Aug 16, 2019 at 10:25:45AM +0800, Zhao Yakui wrote:
> +static
> +int acrn_dev_open(struct inode *inodep, struct file *filep)
> +{
> + pr_info("%s: opening device node\n", __func__);
> +
> + return 0;
> +}
> +
> +static
> +long acrn_dev_ioctl(struct file *filep,
> + unsigned int ioctl_num, unsigned long ioctl_param)
> +{
> + long ret = 0;
> +
> + return ret;


This module is mostly stubs and debugging printks...  :(

I looked ahead in the patch series to see if we do something with the
stubs later on and it turns out we do.  Fold the two patches together so
that we don't have to review patches like this one.  Each patch should
do "one thing" which makes sense and can be reviewed independently.

regards,
dan carpenter

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

Re: [PATCH v2 1/2] binder: Add default binder devices through binderfs when configured

[Dan Carpenter]

On Wed, Aug 07, 2019 at 09:51:46AM -0700, Hridya Valsaraju wrote:
> On Wed, Aug 7, 2019 at 4:02 AM Dan Carpenter  wrote:
> >
> > On Tue, Aug 06, 2019 at 11:40:05AM -0700, Hridya Valsaraju wrote:
> > > @@ -467,6 +466,9 @@ static int binderfs_fill_super(struct super_block 
> > > *sb, void *data, int silent)
> > >   int ret;
> > >   struct binderfs_info *info;
> > >   struct inode *inode = NULL;
> > > + struct binderfs_device device_info = { 0 };
> > > + const char *name;
> > > + size_t len;
> > >
> > >   sb->s_blocksize = PAGE_SIZE;
> > >   sb->s_blocksize_bits = PAGE_SHIFT;
> > > @@ -521,7 +523,24 @@ static int binderfs_fill_super(struct super_block 
> > > *sb, void *data, int silent)
> > >   if (!sb->s_root)
> > >   return -ENOMEM;
> > >
> > > - return binderfs_binder_ctl_create(sb);
> > > + ret = binderfs_binder_ctl_create(sb);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + name = binder_devices_param;
> > > + for (len = strcspn(name, ","); len > 0; len = strcspn(name, ",")) {
> > > + strscpy(device_info.name, name, len + 1);
> > > + ret = binderfs_binder_device_create(inode, NULL, 
> > > _info);
> > > + if (ret)
> > > + return ret;
> >
> > We should probably clean up before returning...  The error handling code
> > would probably be tricky to write though and it's not super common.
> 
> Thank you for taking a look Dan. Did you mean cleaning up the default
> devices that were already created? They will actually be cleaned up by
> binderfs_evict_inode() during the super block's cleanup since the
> mount operation will fail due to an error here.

Yeah.  I meant the binderfs_binder_device_create() from previous
iterations through this loop.

Good to know that it's handled.  Thanks for taking the time to look at
this.

regards,
dan carpenter

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

Re: [PATCH 10/10] staging: rtl8712: r8712_xmit_classifier(): Change return values and type

[Dan Carpenter]

Looks good to me.

Reviewed-by: Dan Carpenter 

regards,
dan carpenter

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

Re: [PATCH 07/10] staging: rtl8712: init_drv_sw(): Change return values

[Dan Carpenter]

On Thu, Aug 08, 2019 at 12:10:09PM +0530, Nishka Dasgupta wrote:
> - if (_r8712_init_sta_priv(>stapriv))
> - return _FAIL;
> + ret = _r8712_init_sta_priv(>stapriv);
> + if (ret)
> + return ret;
>   padapter->stapriv.padapter = padapter;
>   r8712_init_bcmc_stainfo(padapter);
>   r8712_init_pwrctrl_priv(padapter);
>   mp871xinit(padapter);
>   init_default_value(padapter);
>   r8712_InitSwLeds(padapter);
> - return _SUCCESS;
> + return ret;

(Please don't resend.  I'd prefer if someone addressed this in a later
patch).  It's better to "return 0;" here because that's clear without
needing to read back a few lines to see what ret is.

regards,
dan carpenter


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

[bug report] staging: wilc1000: added support to dynamically add/remove interfaces

[Dan Carpenter]

Hello Ajay Singh,

The patch 9bc061e88054: "staging: wilc1000: added support to
dynamically add/remove interfaces" from Jun 26, 2019, leads to the
following static checker warning:

drivers/staging/wilc1000/wilc_wlan.c:497 wilc_wlan_handle_txq()
warn: missing error code here? 'wilc_wlan_txq_get_first()' failed.

drivers/staging/wilc1000/wilc_wlan.c
   474  int wilc_wlan_handle_txq(struct wilc *wilc, u32 *txq_count)
   475  {
   476  int i, entries = 0;
   477  u32 sum;
   478  u32 reg;
   479  u32 offset = 0;
   480  int vmm_sz = 0;
   481  struct txq_entry_t *tqe;
   482  int ret = 0;
^^^

   483  int counter;
   484  int timeout;
   485  u32 vmm_table[WILC_VMM_TBL_SIZE];
   486  const struct wilc_hif_func *func;
   487  u8 *txb = wilc->tx_buffer;
   488  struct net_device *dev;
   489  struct wilc_vif *vif;
   490  
   491  if (wilc->quit)
   492  goto out;
   493  
   494  mutex_lock(>txq_add_to_head_cs);
   495  tqe = wilc_wlan_txq_get_first(wilc);
   496  if (!tqe)
   497  goto out;

Should this really be a success path?

   498  dev = tqe->vif->ndev;
   499  wilc_wlan_txq_filter_dup_tcp_ack(dev);
   500  i = 0;
   501  sum = 0;
   502  do {
   503  if (tqe && (i < (WILC_VMM_TBL_SIZE - 1))) {

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

Re: [PATCH] staging: isdn: hysdn_procconf_init() remove parantheses from return value

[Dan Carpenter]

This driver is going to be deleted soon so we aren't accepting cleanups.

Thanks!

regards,
dan carpenter

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

Re: [PATCH 3/6] staging: wilc1000: remove unused members

[Dan Carpenter]

On Tue, Aug 06, 2019 at 07:32:08PM +, adham.aboza...@microchip.com wrote:
> Hi Dan
> 
> On 8/6/19 5:46 AM, Dan Carpenter wrote:
> > External E-Mail
> >
> >
> > On Thu, Jul 25, 2019 at 09:31:34PM +, adham.aboza...@microchip.com 
> > wrote:
> >> From: Adham Abozaeid 
> >>
> >> remove obtaining_ip from struct wilc_vif
> >>
> > How is this "unused"?  It looks like it is used to me.
> The main usage of obtaining_ip was to track the inetadd_notifier status.
> After removing the notifier and ip address timeout timer in the first and 
> second patch,
> the remaining usage became meaningless, and could be removed.

This is exactly the level of detail that I would like in a commit
description.

regards,
dan carpenter

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

Re: [PATCH v2 1/2] binder: Add default binder devices through binderfs when configured

[Dan Carpenter]

On Tue, Aug 06, 2019 at 11:40:05AM -0700, Hridya Valsaraju wrote:
> @@ -467,6 +466,9 @@ static int binderfs_fill_super(struct super_block *sb, 
> void *data, int silent)
>   int ret;
>   struct binderfs_info *info;
>   struct inode *inode = NULL;
> + struct binderfs_device device_info = { 0 };
> + const char *name;
> + size_t len;
>  
>   sb->s_blocksize = PAGE_SIZE;
>   sb->s_blocksize_bits = PAGE_SHIFT;
> @@ -521,7 +523,24 @@ static int binderfs_fill_super(struct super_block *sb, 
> void *data, int silent)
>   if (!sb->s_root)
>   return -ENOMEM;
>  
> - return binderfs_binder_ctl_create(sb);
> + ret = binderfs_binder_ctl_create(sb);
> + if (ret)
> + return ret;
> +
> + name = binder_devices_param;
> + for (len = strcspn(name, ","); len > 0; len = strcspn(name, ",")) {
> + strscpy(device_info.name, name, len + 1);
> + ret = binderfs_binder_device_create(inode, NULL, _info);
> + if (ret)
> + return ret;

We should probably clean up before returning...  The error handling code
would probably be tricky to write though and it's not super common.

> + name += len;
> + if (*name == ',')
> +     name++;
> +
> + }
> +
> + return 0;
> +

Remove this extra blank line.

>  }

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

Re: [PATCH] Staging: rtl8188eu: core: rtw_security: tidy up crc32_init()

[Dan Carpenter]

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 3/6] staging: wilc1000: remove unused members

[Dan Carpenter]

On Thu, Jul 25, 2019 at 09:31:34PM +, adham.aboza...@microchip.com wrote:
> From: Adham Abozaeid 
> 
> remove obtaining_ip from struct wilc_vif
> 

How is this "unused"?  It looks like it is used to me.

regards,
dan carpenter

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

Re: [PATCH] Staging: rtl8188eu: core: rtw_security: fixed a coding style issue

[Dan Carpenter]

On Tue, Aug 06, 2019 at 05:24:38PM +0530, Merwin Trever Ferrao wrote:
> Fixed WARNING: else is not generally useful after a break or return
> ---

Much better, but you forgot the the Signed-off-by so we can't apply it.

Also it's nice to be more specific with the subject.
[PATCH] Staging: rtl8188eu: rtw_security: tidy up crc32_init().
And for the full commit message maybe write something like:

This code generates a checkpatch warning:

WARNING: else is not generally useful after a break or return

If we move the declarations to the start of the function then we can
pull the code back one tab and it makes the function a lot more readable.

regards,
dan carpenter

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

Re: [PATCH] rtl8712: rtl871x_ioctl_linux.c: fix unnecessary typecast

[Dan Carpenter]

On Mon, Aug 05, 2019 at 10:33:29PM -0300, Jose Carlos Cazarin Filho wrote:
> Fix checkpath warning:
> WARNING: Unnecessary typecast of c90 int constant
> 
> Signed-off-by: Jose Carlos Cazarin Filho 
> ---
>  Hello all!
>  This is my first commit to the Linux Kernel, I'm doing this to learn and be 
> able
>  to contribute more in the future
>  Peace all! 
>  drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c 
> b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> index 944336e0d..da371072e 100644
> --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> @@ -665,8 +665,8 @@ static int r8711_wx_set_freq(struct net_device *dev,
>  
>  /* If setting by frequency, convert to a channel */
>   if ((fwrq->e == 1) &&
> -   (fwrq->m >= (int) 2.412e8) &&
> -   (fwrq->m <= (int) 2.487e8)) {
> +   (fwrq->m >= 2.412e8) &&
> +   (fwrq->m <= 2.487e8)) {

I don't think we can do this.  You're not allowed to use floats in the
kernel (because they make context switching slow).  I could have sworn
that we use the -nofp to stop the compile when people use floats but
this compiles fine for me.

regards,
dan carpenter

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

Re: [PATCH] staging: isdn: remove unnecessary parentheses

[Dan Carpenter]

This driver is obselete so we're just keeping it around for a couple
kernel releases and then deleting it.  We're not taking cleanups for it.

regards,
dan carpenter

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

Re: [PATCH] staging: pi433 line over 80 characters in multiple places

[Dan Carpenter]

The subject needs a colon after "pi433:" but also this change isn't
really an improvement so far as readability goes.

regards,
dan carpenter

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

Re: [PATCH] Fix the following checkpatch warnings:

[Dan Carpenter]

The subject isn't right and I don't really feel that the change makes
the code more beautiful to look at.

regards,
dan carpenter

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

Re: [PATCH] Staging : rtl8188eu : rtw_security.c - Fixed warning: coding style issues - Fixed warning: if statement containing return with an else - Fixed check: coding style issues

[Dan Carpenter]

1) Fix the From header.
2) Fix the subject.
3) Add a blank line after the subject.
4) Split the path up into multiple patches that each do one kind of
   change.

On Mon, Aug 05, 2019 at 01:22:56PM +0530, merwintf wrote:
> Signed-off-by: merwintf 
 
Use your real name like for a legal document.

>  static u8 crc32_reverseBit(u8 data)
>  {
> - return (u8)((data<<7)&0x80) | ((data<<5)&0x40) | ((data<<3)&0x20) |
> -((data<<1)&0x10) | ((data>>1)&0x08) | ((data>>3)&0x04) |
> -((data>>5)&0x02) | ((data>>7)&0x01);
> + return (u8)((data << 7) & 0x80)
> +  | ((data << 5) & 0x40)
> +  | ((data << 3) & 0x20)
> +  | ((data << 1) & 0x10)
> +  | ((data >> 1) & 0x08)
> +  | ((data >> 3) & 0x04)
> +  | ((data >> 5) & 0x02)
> +  | ((data >> 7) & 0x01);


Put the | at the end of the line, not the start.  The cast isn't
required and it kind of messes up the white space so just leave it out
so that we don't have to change this twice.

> + return (u8)((data << 7) & 0x80)
> +  | ((data << 5) & 0x40)
> +  | ((data << 3) & 0x20)
> +  | ((data << 1) & 0x10)
> +  | ((data >> 1) & 0x08)
> +  | ((data >> 3) & 0x04)
> +  | ((data >> 5) & 0x02)
> +  | ((data >> 7) & 0x01);

return ((data << 7) & 0x80) |
   ((data << 5) & 0x40) |

etc.


>  }
>  
>  static void crc32_init(void)
>  {
> - if (bcrc32initialized == 1) {
> - return;
> - } else {
> + if (bcrc32initialized != 1) {

This isn't really an improvement.  Move the declarations outside the
block and do it like this:

int i, j;
u32 c;
u8 *p = (u8 *), *p1;

if (bcrc32initialized == 1)
return;

> @@ -164,7 +172,8 @@ void rtw_wep_encrypt(struct adapter *padapter, u8 
> *pxmitframe)
>   return;
>  
>   if (crypto_ops->set_key(psecuritypriv->dot11DefKey[keyindex].skey,
> - psecuritypriv->dot11DefKeylen[keyindex], NULL, 
> crypto_private) < 0)
> + psecuritypriv->dot11DefKeylen[keyindex],
> + NULL, crypto_private) < 0)
>   goto free_crypto_private;

Introduce an "int ret;" or something.

ret = crypto_ops->set_key();
if (ret < 0)
goto free_crypto_private;




> @@ -201,16 +211,20 @@ void rtw_wep_encrypt(struct adapter *padapter, u8 
> *pxmitframe)
>  
>  int rtw_wep_decrypt(struct adapter  *padapter, u8 *precvframe)
>  {
> - struct  rx_pkt_attrib*prxattrib = &(((struct recv_frame 
> *)precvframe)->attrib);
> + struct  rx_pkt_attrib*prxattrib =
> +   &(((struct recv_frame *)precvframe)->attrib);

This change isn't an improvement.

Anyway, hopefully that gives you some ideas.  But split up the patch.

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

Re: [PATCH] staging: wilc1000: Merge memcpy + le32_to_cpus to get_unaligned_le32

[Dan Carpenter]

On Fri, Jul 19, 2019 at 05:01:33PM +0300, Dan Carpenter wrote:
> On Fri, Jul 19, 2019 at 12:05:07PM +, ajay.kat...@microchip.com wrote:
> > 
> > On 7/19/2019 5:16 PM, Chuhong Yuan wrote:
> > > 
> > >  于2019年7月19日周五 下午7:34写道：
> > >>
> > >> On 7/19/2019 1:40 PM, Chuhong Yuan wrote:
> > >>>
> > >>> Merge the combo use of memcpy and le32_to_cpus.
> > >>> Use get_unaligned_le32 instead.
> > >>> This simplifies the code.
> > >>>
> > >>> Signed-off-by: Chuhong Yuan 
> > >>> ---
> > >>>  drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 3 +--
> > >>>  1 file changed, 1 insertion(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c 
> > >>> b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> > >>> index d72fdd333050..12fb4add05ec 100644
> > >>> --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> > >>> +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> > >>> @@ -1038,8 +1038,7 @@ void wilc_wfi_p2p_rx(struct wilc_vif *vif, u8 
> > >>> *buff, u32 size)
> > >>>   s32 freq;
> > >>>   __le16 fc;
> > >>>
> > >>> - memcpy(, (buff - HOST_HDR_OFFSET), HOST_HDR_OFFSET);
> > >>> - le32_to_cpus();
> > >>> + header = get_unaligned_le32(buff - HOST_HDR_OFFSET);
> > >>>   pkt_offset = GET_PKT_OFFSET(header);
> > >>>
> > >>>   if (pkt_offset & IS_MANAGMEMENT_CALLBACK) {
> > >>>
> > >>
> > >> Thanks for sending the patches.
> > >>
> > >> The code change looks okay to me. Just a minor comment, avoid the use of
> > >> same subject line for different patches.
> > > 
> > > These two patches are in the same subsystem and solve the same problem.
> > > I splitted them into two patches by mistake since I did not notice the 
> > > problems
> > > in the second patch when I sent the first one.
> > > Should I merge the two patches and resend?
> > > 
> > 
> > Yes, please go ahead, merge the patches and send the updated version.
> > 
> 
> This is wrong advice.  Don't merge the patches because they are for
> different drivers.  The original subjects are fine because the
> subsystems are different so that's okay.
> 

Oh...  My bad...  I was looking at the wrong patches.  :P  You are
100% correct Ajay.  Merge the two patches and always make sure to not
send multiple patches with the same subject.

regards,
dan carpenter

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

Re: [PATCH] staging: wilc1000: Merge memcpy + le32_to_cpus to get_unaligned_le32

[Dan Carpenter]

On Fri, Jul 19, 2019 at 12:05:07PM +, ajay.kat...@microchip.com wrote:
> 
> On 7/19/2019 5:16 PM, Chuhong Yuan wrote:
> > 
> >  于2019年7月19日周五 下午7:34写道：
> >>
> >> On 7/19/2019 1:40 PM, Chuhong Yuan wrote:
> >>>
> >>> Merge the combo use of memcpy and le32_to_cpus.
> >>> Use get_unaligned_le32 instead.
> >>> This simplifies the code.
> >>>
> >>> Signed-off-by: Chuhong Yuan 
> >>> ---
> >>>  drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 3 +--
> >>>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c 
> >>> b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> >>> index d72fdd333050..12fb4add05ec 100644
> >>> --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> >>> +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> >>> @@ -1038,8 +1038,7 @@ void wilc_wfi_p2p_rx(struct wilc_vif *vif, u8 
> >>> *buff, u32 size)
> >>>   s32 freq;
> >>>   __le16 fc;
> >>>
> >>> - memcpy(, (buff - HOST_HDR_OFFSET), HOST_HDR_OFFSET);
> >>> - le32_to_cpus();
> >>> + header = get_unaligned_le32(buff - HOST_HDR_OFFSET);
> >>>   pkt_offset = GET_PKT_OFFSET(header);
> >>>
> >>>   if (pkt_offset & IS_MANAGMEMENT_CALLBACK) {
> >>>
> >>
> >> Thanks for sending the patches.
> >>
> >> The code change looks okay to me. Just a minor comment, avoid the use of
> >> same subject line for different patches.
> > 
> > These two patches are in the same subsystem and solve the same problem.
> > I splitted them into two patches by mistake since I did not notice the 
> > problems
> > in the second patch when I sent the first one.
> > Should I merge the two patches and resend?
> > 
> 
> Yes, please go ahead, merge the patches and send the updated version.
> 

This is wrong advice.  Don't merge the patches because they are for
different drivers.  The original subjects are fine because the
subsystems are different so that's okay.

regards,
dan carpenter

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

Re: [OSSNA] Intro to kernel hacking tutorial

[Dan Carpenter]

On Fri, Jul 05, 2019 at 12:50:55PM +1000, Tobin C. Harding wrote:
> Outcome will (hopefully) be a small patch set into drivers/staging/.
> (Don't worry Greg only one group got to this stage last time, you
> won't get flooded with patches :)

We're good at reviewing floods of patches.  Send away.

In the end what we want is people who will take over a driver and
understand it completely and become the maintainer.  We've had a few
people that did appointed themselves to become the maintainer of a
random driver and move it out of staging.  But even if people don't make
it all the way to become a maintainer, it's nice when they start down
that path by focusing on one driver and trying to understand it as much
as possible.

Most of the time when you look at a new staging driver, then you do want
to clean up the white space just because it's hard to look at
non-standard code.  So that's the first step.  But then maybe start at
the probe and release functions and clean it up.  Keep your eyes open
to any other mistakes or bugs you see.  Write them down.  Then the
ioctls.  Etc.  Look at the TODO too.

The other thing I wish people knew was about the relationship with
maintainers.  When you start out, you're virtually anonymous for the
first couple patchsets.  We get so many and they blend together so we
don't remember your name.  So don't think that we mean anything
personally if we don't apply your patch.  We have forgotten about the
patch as soon as we reply to it.  Don't panic and resend quickly.  You
will be too stressed.  Wait until the next day.

In staging we really want to apply patches (unless it's in staging
because we're going to remove the code).  I get annoyed with other
staging reviewers who NAK patches because "I don't like churn" or
whatever.

On the other hand, patches just "silencing checkpatch.pl" is not a valid
justification for sending a patch.  Patches should make the code more
readable.

Anyway, maintainers are not monsters.  Very few people have made me
annoyed to the point where I refuse to review their code.  And everyone
else is in my good books so that's fine.

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

Re: [PATCH 1/2] staging: rts5208: Rewrite redundant if statement to improve code style

[Dan Carpenter]

On Sun, Jun 30, 2019 at 04:12:44PM +0200, Tobias Nießen wrote:
> Am 26.06.2019 um 16:56 schrieb Dan Carpenter:
> > Both these patches seem fine.
> > 
> > On Wed, Jun 26, 2019 at 04:28:56PM +0200, Tobias Nießen wrote:
> >> This commit uses the fact that
> >>
> >> if (a) {
> >> if (b) {
> >> ...
> >> }
> >> }
> >>
> >> can instead be written as
> >>
> >> if (a && b) {
> >> ...
> >> }
> >>
> >> without any change in behavior, allowing to decrease the indentation
> >> of the contained code block and thus reducing the average line length.
> >>
> >> Signed-off-by: Tobias Nießen 
> >> Signed-off-by: Sabrina Gaube 
> > 
> > Signed-off-by is like signing a legal document that you didn't put any
> > of SCO's secret UNIXWARE source code into your patch or do other illegal
> > activities.  Everyone who handles a patch is supposed to Sign it.
> > 
> > It's weird to see Sabrina randomly signing your patches.  Probably there
> > is a more appropriate kind of tag to use as well or instead such as
> > Co-Developed-by, Reviewed-by or Suggested-by.
> > 
> > regards,
> > dan carpenter
> > 
> 
> Thank you, Dan. This patch series is a mandatory part of a course
> Sabrina and I are taking at university. We were told to add
> Signed-off-by for both of us. I can add Co-Developed-by if that helps?

Yes.  It does help.

regards,
dan carpenter

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

Re: [PATCH 01/10] staging/rtl8723bs/hal: fix comparison to true/false is error prone

[Dan Carpenter]

You sent 10 patches with the same subject.

Btw, I can't recall ever seeing a bug caused by a true false comparison.
I agree on style principles with the checkpatch warning, but I do
think it over states the risk (which is as far as I can see is zero).

regards,
dan carpenter

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

Re: [PATCH 29/31] staging: bcm2835-camera: Add sanity checks for queue_setup/CREATE_BUFS

[Dan Carpenter]

On Thu, Jun 27, 2019 at 11:09:27PM +0200, Stefan Wahren wrote:
> From: Dave Stevenson 
> 
> Fixes a v4l2-compliance failure when passed a buffer that is
> too small.
> queue_setup wasn't handling the case where !(*nplanes), as
 ^^^
This is reversed?  It wasn't handling where *nplanes is non-zero.

> used from CREATE_BUFS and requiring the driver to sanity
> check the provided buffer parameters. It was assuming that
> it was always being used in the REQBUFS case where it provides
> the buffer properties.

These patches look really nice.

regards,
dan carpenter


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

Re: [PATCH 06/31] staging: bcm2835-camera: Return early on errors

[Dan Carpenter]

On Thu, Jun 27, 2019 at 08:56:03PM +0200, Stefan Wahren wrote:
>   v4l2_dbg(1, bcm2835_v4l2_debug, >v4l2_dev, "connecting %p to %p\n",
>src, dst);
>   ret = vchiq_mmal_port_connect_tunnel(dev->instance, src, dst);
>   if (!ret)
>   ret = vchiq_mmal_port_enable(dev->instance, src, NULL);
> -error:
> +
>   return ret;

In future patches, you probably want to flip this one around as well.
Try to do error handling instead of success handling.  In other words,
keep the success patch indented one tab and the failure path indented
two tabs.  Don't make the last failure check in the function special.

ret = vchiq_mmal_port_connect_tunnel(dev->instance, src, dst);
if (ret)
return ret;

ret = vchiq_mmal_port_enable(dev->instance, src, NULL);
if (ret)
return ret;
return 0;

Or you can make the last check a little special if you want...

return vchiq_mmal_port_enable(dev->instance, src, NULL);

Either format is good.

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

Re: [PATCH 01/31] staging: bcm2835-camera: Ensure H264 header bytes get a sensible timestamp

[Dan Carpenter]

On Thu, Jun 27, 2019 at 08:55:58PM +0200, Stefan Wahren wrote:
> diff --git a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.h 
> b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.h
> index 2b5679e..09273b0 100644
> --- a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.h
> +++ b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.h
> @@ -90,6 +90,8 @@ struct bm2835_mmal_dev {
>   s64 vc_start_timestamp;
>   /* Kernel start timestamp for streaming */
>   ktime_t kernel_start_ts;
> + /* Timestamp of last frame */
> + u64 last_timestamp;

Not directly related to this patch but the indenting in this .h file is
all higgle-piggledy.

regards,
dan carpenter

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

Re: [PATCH] media: bcm2048: Macros with complex values should be enclosed in parentheses

[Dan Carpenter]

This breaks the build.  :(

regards,
dan carpenter

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

Re: [PATCH] fbtft: Cleanup line over 80 character warnings

[Dan Carpenter]

Sorry, I don't feel like this makes it more readable.

regards,
dan carpenter

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

Re: [PATCH] media: staging/imx: Fix NULL deref in find_pipeline_entity()

[Dan Carpenter]

On Wed, Jun 26, 2019 at 11:52:25AM -0700, Steve Longerbeam wrote:
> Fix a cut error in find_pipeline_entity(). The start entity must be
> passed to media_entity_to_video_device() in find_pipeline_entity(), not
> pad->entity. The pad is only put to use later, after determining the start
> entity is not the entity being searched for.
> 
> Fixes: 3ef46bc97ca2 ("media: staging/imx: Improve pipeline searching")
> 
> Reported-by: Colin Ian King 
> Signed-off-by: Steve Longerbeam 
> ---
>  drivers/staging/media/imx/imx-media-utils.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/imx/imx-media-utils.c 
> b/drivers/staging/media/imx/imx-media-utils.c
> index b5b8a3b7730a..6fb88c22ee27 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -842,7 +842,7 @@ find_pipeline_entity(struct media_entity *start, u32 
> grp_id,
>   if (sd->grp_id & grp_id)
>   return >entity;
>   } else if (buftype && is_media_entity_v4l2_video_device(start)) {
> - vfd = media_entity_to_video_device(pad->entity);
> + vfd = media_entity_to_video_device(start);

Can we also remove the "pad = NULL" assignment at the start of the
function?  Otherwise static checkers and new versions of GCC will warn
that the assignment isn't used.  Plus removing the initialization will
prevent bugs like this in the future.

regards,
dan carpenter

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

Re: [PATCH] staging: rtl8723bs: hal: sdio_halinit: Remove set but unused varilable pHalData

[Dan Carpenter]

On Wed, Jun 26, 2019 at 11:14:59PM +0530, Hariprasad Kelam wrote:
> @@ -1433,7 +1430,6 @@ static void SetHwReg8723BS(struct adapter *padapter, u8 
> variable, u8 *val)
>  #endif
>  #endif
>  
> - pHalData = GET_HAL_DATA(padapter);
>  
>   switch (variable) {

We need to delete one of those blank lines or it introduces a new
checkpatch warning.

regards,
dan carpenter

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

Re: [PATCH V2] staging: bcm2835-camera: Restore return behavior of ctrl_set_bitrate()

[Dan Carpenter]

Thanks!

Acked-by: Dan Carpenter 

regards,
dan carpenter

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

Re: [PATCH 1/2] staging: rts5208: Rewrite redundant if statement to improve code style

[Dan Carpenter]

Both these patches seem fine.

On Wed, Jun 26, 2019 at 04:28:56PM +0200, Tobias Nießen wrote:
> This commit uses the fact that
> 
> if (a) {
> if (b) {
> ...
> }
> }
> 
> can instead be written as
> 
> if (a && b) {
> ...
> }
> 
> without any change in behavior, allowing to decrease the indentation
> of the contained code block and thus reducing the average line length.
> 
> Signed-off-by: Tobias Nießen 
> Signed-off-by: Sabrina Gaube 

Signed-off-by is like signing a legal document that you didn't put any
of SCO's secret UNIXWARE source code into your patch or do other illegal
activities.  Everyone who handles a patch is supposed to Sign it.

It's weird to see Sabrina randomly signing your patches.  Probably there
is a more appropriate kind of tag to use as well or instead such as
Co-Developed-by, Reviewed-by or Suggested-by.

regards,
dan carpenter

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

Re: [PATCH 0/8] staging: kpc2000: style refactoring

[Dan Carpenter]

This is better.

Reviewed-by: Dan Carpenter 

regards,
dan carpenter

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

Re: [PATCH] staging: comedi: use dma_mmap_coherent for DMA-able buffer mmap

[Dan Carpenter]

On Tue, Jun 25, 2019 at 02:21:41PM +0100, Ian Abbott wrote:
> On 25/06/2019 12:47, Dan Carpenter wrote:
> > On Tue, Jun 25, 2019 at 12:26:59PM +0100, Ian Abbott wrote:
> > > + } else {
> > > + for (i = 0; i < bm->n_pages; i++) {
> > > + buf = >page_list[i];
> > > + ClearPageReserved(virt_to_page(buf->virt_addr));
> > 
> > I think we need a NULL check here:
> > 
> > if (!buf->virt_addr)
> > continue;
> > 
> > >   free_page((unsigned 
> > > long)buf->virt_addr);
> > >   }
> > >   }
> 
> Hi Dan, I don't think that is strictly required because bm->n_pages gets set
> to the number of successfully allocated pages (not the number of required
> pages) by comedi_buf_map_alloc():
> 
> > +   for (i = 0; i < n_pages; i++) {
> > +   buf = >page_list[i];
> > +   buf->virt_addr = (void *)get_zeroed_page(GFP_KERNEL);
> > +   if (!buf->virt_addr)
> > +   break;
> > +
> > +   SetPageReserved(virt_to_page(buf->virt_addr));
> > +   }
> > +
> > +   bm->n_pages = i;
> 
> Here!   ^
> 

Oh, yeah.  I misread.  Sorry for the noise.

regards,
dan carpenter


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

Re: [PATCH 6/8] staging: kpc2000: introduce 'unsigned int'

[Dan Carpenter]

On Tue, Jun 25, 2019 at 01:27:17PM +0200, Fabian Krueger wrote:
> Replaced 'unsigned' with it's equivalent 'unsigned int' to reduce
> confusion while reading the code.
> 
> Signed-off-by: Fabian Krueger 
> Signed-off-by: Michael Scheiderer 
> ---
>  drivers/staging/kpc2000/kpc2000_spi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/kpc2000/kpc2000_spi.c 
> b/drivers/staging/kpc2000/kpc2000_spi.c
> index 79d7c44315e8..732254e9b261 100644
> --- a/drivers/staging/kpc2000/kpc2000_spi.c
> +++ b/drivers/staging/kpc2000/kpc2000_spi.c
> @@ -337,7 +337,7 @@ kp_spi_transfer_one_message(struct spi_master *master, 
> struct spi_message *m)
>   list_for_each_entry(transfer, >transfers, transfer_list) {
>   const void *tx_buf = transfer->tx_buf;
>   void   *rx_buf = transfer->rx_buf;
> - unsignedlen = transfer->len;
> + unsigned int   len = transfer->len;
    
This white space isn't correct.

regards,
dan carpenter

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

Re: [PATCH 4/8] staging: kpc2000: add spaces

[Dan Carpenter]

On Tue, Jun 25, 2019 at 01:27:15PM +0200, Fabian Krueger wrote:
> Added spaces on the left side of parenthesis and on both sides of binary
> operators.
> This refactoring makes the code more readable.
> 
> Signed-off-by: Fabian Krueger 
> Signed-off-by: Michael Scheiderer 
> ---
>  drivers/staging/kpc2000/kpc2000_spi.c | 15 ++-
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/kpc2000/kpc2000_spi.c 
> b/drivers/staging/kpc2000/kpc2000_spi.c
> index 253a9c150d33..8f56886f4673 100644
> --- a/drivers/staging/kpc2000/kpc2000_spi.c
> +++ b/drivers/staging/kpc2000/kpc2000_spi.c
> @@ -192,9 +192,8 @@ 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))
>       return cs->conf_cache;
> - }

This doesn't match the patch description.

regards,
dan carpenter

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

Re: [PATCH 1/8] staging: kpc2000: add line breaks

[Dan Carpenter]

On Tue, Jun 25, 2019 at 01:27:12PM +0200, Fabian Krueger wrote:
> diff --git a/drivers/staging/kpc2000/kpc2000_spi.c 
> b/drivers/staging/kpc2000/kpc2000_spi.c
> index c3e5c1848f53..7ed0fb6b4abb 100644
> --- a/drivers/staging/kpc2000/kpc2000_spi.c
> +++ b/drivers/staging/kpc2000/kpc2000_spi.c
> @@ -30,18 +30,46 @@
>  #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},
> +
> + {.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},
>  };

The original is fine/better.

> @@ -215,7 +244,9 @@ kp_spi_txrx_pio(struct spi_device *spidev, struct 
> spi_transfer *transfer)
>   for (i = 0 ; i < c ; i++) {
>   char val = *tx++;
>  
> - if (kp_spi_wait_for_reg_bit(cs, KP_SPI_REG_STATUS, 
> KP_SPI_REG_STATUS_TXS) < 0) {
> + if (kp_spi_wait_for_reg_bit(cs, KP_SPI_REG_STATUS,
> + KP_SPI_REG_STATUS_TXS)
> + < 0) {

I don't like how the < 0 is on the next line.  It would be better to
introduce an "int ret;"
ret = kp_spi_wait_for_reg_bit(cs, KP_SPI_REG_STATUS,
  KP_SPI_REG_STATUS_TXS);
if (ret < 0)
goto out;


>   goto out;
>   }
>  

> @@ -317,7 +353,8 @@ kp_spi_transfer_one_message(struct spi_master *master, 
> struct spi_message *m)
>   dev_dbg(kpspi->dev, "  transfer -EINVAL\n");
>   return -EINVAL;
>   }
> - if (transfer->speed_hz && (transfer->speed_hz < (KP_SPI_CLK >> 
> 15))) {
> + if (transfer->speed_hz && (transfer->speed_hz <
> +(KP_SPI_CLK >> 15))) {


Move the whole conition down:

if (transfer->speed_hz &&
transfer->speed_hz < (KP_SPI_CLK >> 15)) {

regards,
dan carpenter

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

Re: [PATCH] staging: comedi: use dma_mmap_coherent for DMA-able buffer mmap

[Dan Carpenter]

On Tue, Jun 25, 2019 at 12:26:59PM +0100, Ian Abbott wrote:
>  drivers/staging/comedi/comedi_buf.c  | 150 ++-
>  drivers/staging/comedi/comedi_fops.c |  39 ---
>  2 files changed, 125 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/staging/comedi/comedi_buf.c 
> b/drivers/staging/comedi/comedi_buf.c
> index d2c8cc72a99d..3ef3ddabf139 100644
> --- a/drivers/staging/comedi/comedi_buf.c
> +++ b/drivers/staging/comedi/comedi_buf.c
> @@ -27,18 +27,19 @@ static void comedi_buf_map_kref_release(struct kref *kref)
>   unsigned int i;
>  
>   if (bm->page_list) {
> - for (i = 0; i < bm->n_pages; i++) {
> - buf = >page_list[i];
> - clear_bit(PG_reserved,
> -   &(virt_to_page(buf->virt_addr)->flags));
> - if (bm->dma_dir != DMA_NONE) {
> -#ifdef CONFIG_HAS_DMA
> - dma_free_coherent(bm->dma_hw_dev,
> -   PAGE_SIZE,
> -   buf->virt_addr,
> -   buf->dma_addr);
> -#endif
> - } else {
> + if (bm->dma_dir != DMA_NONE) {
> + /*
> +  * DMA buffer was allocated as a single block.
> +  * Address is in page_list[0].
> +  */
> + buf = >page_list[0];
> + dma_free_coherent(bm->dma_hw_dev,
> +   PAGE_SIZE * bm->n_pages,
> +   buf->virt_addr, buf->dma_addr);
> + } else {
> + for (i = 0; i < bm->n_pages; i++) {
> + buf = >page_list[i];
> + ClearPageReserved(virt_to_page(buf->virt_addr));

I think we need a NULL check here:

if (!buf->virt_addr)
continue;

>       free_page((unsigned long)buf->virt_addr);
>   }
>   }
> @@ -57,7 +58,8 @@ static void __comedi_buf_free(struct comedi_device *dev,

regards,
dan carpenter

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

Re: [PATCH] staging: bcm2835-camera: Restore return behavior of ctrl_set_bitrate()

[Dan Carpenter]

On Tue, Jun 25, 2019 at 12:13:15AM +0200, Stefan Wahren wrote:
> The commit 52c4dfcead49 ("Staging: vc04_services: Cleanup in
> ctrl_set_bitrate()") changed the return behavior of ctrl_set_bitrate().
> This breaks probing of bcm2835-camera:
> 
> bcm2835-v4l2: mmal_init: failed to set all camera controls: -3
> Cleanup: Destroy video encoder
> Cleanup: Destroy image encoder
> Cleanup: Destroy video render
> Cleanup: Destroy camera
> bcm2835-v4l2: bcm2835_mmal_probe: mmal init failed: -3
> bcm2835-camera: probe of bcm2835-camera failed with error -3
> 
> So restore the old behavior and fix this issue.
> 
> Fixes: 52c4dfcead49 ("Staging: vc04_services: Cleanup in ctrl_set_bitrate()")
> Signed-off-by: Stefan Wahren 

I feel like this papers over the issue.  It would be better to figure
out why this is failing and fix it properly.  -3 is -ESRCH and when I
grep for ESRCH I only see it used in the ioctl so that can't be it.

I think it must be -MMAL_MSG_STATUS_EINVAL actually, but it comes from
the firmware or something so we can't grep for it.

Can we do some more digging to find out why it's failing or otherwise
we could add a comment.

/*
 * FIXME:  port_parameter_set() sometimes fails with
 * -MMAL_MSG_STATUS_EINVAL and we don't know why so we're
 * ignoring those errors for now.
 *
 */
return 0;


regards,
dan carpenter

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

Re: [PATCH] staging: rtl8723bs: hal: hal_btcoex: Remove variables pHalData and pU1Tmp

[Dan Carpenter]

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: kpc2000: simplify error handling in kp2000_pcie_probe

[Dan Carpenter]

On Wed, Jun 19, 2019 at 08:36:07AM +0200, Simon Sandström wrote:
> We can get rid of a few iounmaps in the middle of the function by
> re-ordering the error handling labels and adding two new labels.
> 
> Signed-off-by: Simon Sandström 
> ---
> 
> This change has not been tested besides by compiling. It might be good
> took take an extra look to make sure that I got everything right.
> 

You have the right instincts that when something looks really
complicated that's probably for a reason.  That attitude will serve you
well in the future!  But in this case it's staging code so the original
code is just strange.

Reviewed-by: Dan Carpenter 

> Also, this change was proposed by Dan Carpenter. Should I add anything
> in the commit message to show this?

There is a Suggested-by: tag for this, but don't resend because I don't
care and I've already reviewed this version so I don't want to review
the patch again.

regards,
dan carpenter

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

Re: kernel BUG at drivers/android/binder_alloc.c:LINE! (4)

[Dan Carpenter]

It's weird that that binder_alloc_copy_from_buffer() is a void function.
It would be easier to do the error handling at that point, instead of in
the callers.  It feels like we keep hitting similar bugs to this.

regards,
dan carpenter

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

Re: [Patch v2] staging: rtl8723bs: os_dep: ioctl_linux: make use of kzalloc

[Dan Carpenter]

On Tue, Jun 18, 2019 at 07:14:10AM +0530, Hariprasad Kelam wrote:
> kmalloc with memset can be replaced with kzalloc.
> 
> Signed-off-by: Hariprasad Kelam 
> -
> changes in v2: Replace rtw_zmalloc with kzalloc
> ---
> ---


The changelog should say something like:

This patch is a cleanup which replaces rtw_malloc(wep_total_len)
with kzalloc() and removes the memset().

The rtw_malloc() does GFP_ATOMIC allocations when in_atomic() is true.
But as the comments for in_atomic() describe, the in_atomic() check
should not be used in driver code.  The in_atomic() check is not
accurate when preempt is disabled.

In this code we are not in IRQ context and we are not holding any
spin_locks so GFP_KERNEL is safe.

regards,
dan carpenter

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

Re: [PATCH] staging: iio: ad7150: use ternary operating to ensure 0/1 value

[Dan Carpenter]

On Sun, Jun 16, 2019 at 11:15:16AM +0100, Jonathan Cameron wrote:
> On Fri, 14 Jun 2019 13:50:59 -0300
> Melissa Wen  wrote:
> 
> > Remove idiom and use ternary operator for consistently trigger 0/1 value
> > on variable declaration.
> > 
> > Signed-off-by: Melissa Wen 
> Hi Melissa,
> 
> In general I would consider this unnecessary churn as, whilst
> it's no longer a favoured idiom, it is extremely common in the
> kernel.

It's still my favourite...  Why wouldn't people like it?  It feels like
last week I just saw someone send a bunch of:

-   foo = (bar == baz) ? 1 : 0;
+   foo = (bar == baz);

patches and I thought it was an improvement at the time...

regards,
dan carpenter

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

Re: use exact allocation for dma coherent memory

[Dan Carpenter]

I once wrote a Smatch check based on a commit message that said we can't
pass dma_alloc_coherent() pointers to virt_to_phys().  But then I never
felt like I understood the rules enough to actually report the warnings
as bugs.

drivers/platform/x86/dcdbas.c:108 smi_data_buf_realloc() error: 'buf' came from 
dma_alloc_coherent() so we can't do virt_to_phys()
drivers/net/caif/caif_virtio.c:414 cfv_create_genpool() error: 
'cfv->alloc_addr' came from dma_alloc_coherent() so we can't do virt_to_phys()
drivers/infiniband/hw/cxgb4/qp.c:135 alloc_host_sq() error: 'sq->queue' came 
from dma_alloc_coherent() so we can't do virt_to_phys()
drivers/infiniband/hw/cxgb4/qp.c:272 create_qp() error: 'wq->rq.queue' came 
from dma_alloc_coherent() so we can't do virt_to_phys()
drivers/infiniband/hw/cxgb4/qp.c:2628 alloc_srq_queue() error: 'wq->queue' came 
from dma_alloc_coherent() so we can't do virt_to_phys()
drivers/infiniband/hw/ocrdma/ocrdma_verbs.c:494 ocrdma_alloc_ucontext() error: 
'ctx->ah_tbl.va' came from dma_alloc_coherent() so we can't do virt_to_phys()

drivers/infiniband/hw/cxgb4/qp.c
   129  static int alloc_host_sq(struct c4iw_rdev *rdev, struct t4_sq *sq)
   130  {
   131  sq->queue = dma_alloc_coherent(&(rdev->lldi.pdev->dev), 
sq->memsize,
   132 &(sq->dma_addr), GFP_KERNEL);
   133  if (!sq->queue)
   134  return -ENOMEM;
   135  sq->phys_addr = virt_to_phys(sq->queue);
   136  dma_unmap_addr_set(sq, mapping, sq->dma_addr);
   137      return 0;
   138  }

Is this a bug?

regards,
dan carpenter

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

Re: [PATCH] staging: rtl8723bs: os_dep: ioctl_linux: Make use rtw_zmalloc

[Dan Carpenter]

On Sun, Jun 16, 2019 at 11:02:50AM +0530, Hariprasad Kelam wrote:
> rtw_malloc with memset can be replace with rtw_zmalloc.
> 
> Signed-off-by: Hariprasad Kelam 
> ---
>  drivers/staging/rtl8723bs/os_dep/ioctl_linux.c | 12 +++-
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c 
> b/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
> index fc3885d..c59e366 100644
> --- a/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
> +++ b/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
> @@ -478,14 +478,12 @@ static int wpa_set_encryption(struct net_device *dev, 
> struct ieee_param *param,
>   if (wep_key_len > 0) {
>   wep_key_len = wep_key_len <= 5 ? 5 : 13;
>   wep_total_len = wep_key_len + FIELD_OFFSET(struct 
> ndis_802_11_wep, KeyMaterial);
> - pwep = rtw_malloc(wep_total_len);
> + pwep = rtw_zmalloc(wep_total_len);

We should not introduce new uses of rtw_malloc() or rtw_zmalloc().  They
are buggy garbage.  Use normall kmalloc() and kzalloc().

regards,
dan carpenter

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

Re: [PATCH V2 08/15] staging: unisys: visorhba: use sg helper to operate sgl

[Dan Carpenter]

On Thu, Jun 13, 2019 at 06:04:11PM +0800, Ming Lei wrote:
> On Thu, Jun 13, 2019 at 11:52:14AM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Jun 13, 2019 at 03:13:28PM +0800, Ming Lei wrote:
> > > The current way isn't safe for chained sgl, so use sg helper to
> > > operate sgl.
> > 
> > I can not make any sense out of this changelog.
> > 
> > What "isn't safe"?  What is a "sgl"?
> 
> sgl is 'scatterlist' in kernel, and several linear sgl can be chained
> together, so accessing the sgl in linear way may see a chained sg, which
> is like a link pointer, then may cause trouble for driver.
> 

So from a user perspective it results in an Oops?  It would be really
cool if you had the copy of the Oops btw so people could grep the git
history for it.

(You need to resend with the improved commit message).

regards,
dan carpenter

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

Re: [PATCH 01/11] staging: rtl8723bs: core: Remove function eeprom_read_sz()

[Dan Carpenter]

On Thu, Jun 13, 2019 at 01:53:20PM +0530, Nishka Dasgupta wrote:
> On 13/06/19 12:15 PM, Dan Carpenter wrote:
> > On Wed, Jun 12, 2019 at 11:34:29PM +0530, Nishka Dasgupta wrote:
> > > Remove unused function eeprom_read_sz.
> > > Issue found with Coccinelle.
> > > 
> > > Signed-off-by: Nishka Dasgupta 
> > 
> > This is great but you need to remove the declaration from the .h file
> > as well.  I noticed some of the other patches have this problem as well
> > so please check them and resend the whole set.
> 
> I'm sorry, I couldn't find the declaration in any .h file for any of these
> patches, even after fetch origin, rebase, and grep. Going to individual .h
> files and searching for declarations does not seem to work either. Is there
> any other way I can look for the declarations?
> 

Oh...  Heh.  Sorry for the noise.  My bad.

I was looking at the wrong driver.  It's declared but not implemented in
rtl8188eu in drivers/staging/rtl8188eu/include/rtw_eeprom.h.  We should
delete those too, but it's unrelated to your patchset so don't worry
about it.

regards,
dan carpenter

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

Re: [PATCH 01/11] staging: rtl8723bs: core: Remove function eeprom_read_sz()

[Dan Carpenter]

On Wed, Jun 12, 2019 at 11:34:29PM +0530, Nishka Dasgupta wrote:
> Remove unused function eeprom_read_sz.
> Issue found with Coccinelle.
> 
> Signed-off-by: Nishka Dasgupta 

This is great but you need to remove the declaration from the .h file
as well.  I noticed some of the other patches have this problem as well
so please check them and resend the whole set.

regards,
dan carpenter

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

Re: [PATCH] media: imx7-media-csi: get csi upstream endpoint

[Dan Carpenter]

On Tue, Jun 11, 2019 at 04:09:55PM +0100, Rui Miguel Silva wrote:
> When the upstream endpoint is neither a mux nor a CSI2 module, just get
> the source pad directly upstream from the CSI.
> 
> Fixes: 05f634040c0d ("media: staging/imx7: add imx7 CSI subdev driver")
> Reported-by: Sebastien Szymanski 
> Signed-off-by: Rui Miguel Silva 
> ---
>  drivers/staging/media/imx/imx7-media-csi.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c 
> b/drivers/staging/media/imx/imx7-media-csi.c
> index 9101566f3f67..8979ee0c8202 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -442,6 +442,14 @@ static int imx7_csi_get_upstream_endpoint(struct 
> imx7_csi *csi,
>  
>   src = >src_sd->entity;
>  
> + /*
> +  * if the source in neither a mux or csi2 get the one directly upstream
 ^^
is?

> +  * from this csi
> +  */
> + if (src->function != MEDIA_ENT_F_VID_IF_BRIDGE &&
> + src->function != MEDIA_ENT_F_VID_MUX)
> + src = >sd.entity;

This would be easier to read if the white space were tweaked a little:

if (src->function != MEDIA_ENT_F_VID_IF_BRIDGE &&
src->function != MEDIA_ENT_F_VID_MUX)
src = >sd.entity;

regards,
dan carpenter


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

Re: [PATCH 2/2] staging: kpc2000: remove unnecessary comments in kp2000_pcie_probe

[Dan Carpenter]

On Mon, Jun 10, 2019 at 10:05:35PM +0200, Simon Sandström wrote:
> @@ -349,9 +340,7 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
>   goto err_remove_ida;
>   }
>  
> - /*
> -  * Step 4: Setup the Register BAR
> -  */
> + // Setup the Register BAR

Greg, are we moving the C++ style comments?  Linus is fine with them.  I
don't like them but whatever...

>   reg_bar_phys_addr = pci_resource_start(pcard->pdev, REG_BAR);
>   reg_bar_phys_len = pci_resource_len(pcard->pdev, REG_BAR);
>  

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

Re: [PATCH 1/2] staging: kpc2000: improve label names in kp2000_pcie_probe

[Dan Carpenter]

Thanks!

Reviewed-by: Dan Carpenter 

Not related to your patch (IOW ignore if you want to) the error handling
is slightly more complicated than required:

drivers/staging/kpc2000/kpc2000/core.c
   356   * Step 4: Setup the Register BAR
   357   */
   358  reg_bar_phys_addr = pci_resource_start(pcard->pdev, REG_BAR);
   359  reg_bar_phys_len = pci_resource_len(pcard->pdev, REG_BAR);
   360  
   361  pcard->regs_bar_base = ioremap_nocache(reg_bar_phys_addr, 
PAGE_SIZE);
   362  if (!pcard->regs_bar_base) {
   363  dev_err(>pdev->dev,
   364  "probe: REG_BAR could not remap memory to 
virtual space\n");
   365  err = -ENODEV;
   366  goto err_disable_device;
   367  }
   368  dev_dbg(>pdev->dev,
   369  "probe: REG_BAR virt hardware address start [%p]\n",
   370  pcard->regs_bar_base);
   371  
   372  err = pci_request_region(pcard->pdev, REG_BAR, 
KP_DRIVER_NAME_KP2000);
   373  if (err) {
   374  iounmap(pcard->regs_bar_base);

We could move this to the bottom of the function.  We would need to
re-order it slightly to free things in the mirror of how they are
allocated.  (Always just free the most recent allocation).

   375  dev_err(>pdev->dev,
   376  "probe: failed to acquire PCI region (%d)\n",
   377  err);
   378  err = -ENODEV;
   379  goto err_disable_device;
   380  }
   381  
   382  pcard->regs_base_resource.start = reg_bar_phys_addr;
   383  pcard->regs_base_resource.end   = reg_bar_phys_addr +
   384reg_bar_phys_len - 1;
   385  pcard->regs_base_resource.flags = IORESOURCE_MEM;
   386  
   387  /*
   388   * Step 5: Setup the DMA BAR
   389   */
   390  dma_bar_phys_addr = pci_resource_start(pcard->pdev, DMA_BAR);
   391  dma_bar_phys_len = pci_resource_len(pcard->pdev, DMA_BAR);
   392  
   393  pcard->dma_bar_base = ioremap_nocache(dma_bar_phys_addr,
   394dma_bar_phys_len);
   395  if (!pcard->dma_bar_base) {
   396  dev_err(>pdev->dev,
   397  "probe: DMA_BAR could not remap memory to 
virtual space\n");
   398  err = -ENODEV;
   399  goto err_unmap_regs;
   400  }
   401  dev_dbg(>pdev->dev,
   402  "probe: DMA_BAR virt hardware address start [%p]\n",
   403  pcard->dma_bar_base);
   404  
   405  pcard->dma_common_regs = pcard->dma_bar_base + 
KPC_DMA_COMMON_OFFSET;
   406  
   407  err = pci_request_region(pcard->pdev, DMA_BAR, "kp2000_pcie");
   408  if (err) {
   409  iounmap(pcard->dma_bar_base);

Same.

   410  dev_err(>pdev->dev,
   411  "probe: failed to acquire PCI region (%d)\n", 
err);
   412  err = -ENODEV;
   413  goto err_unmap_regs;
   414  }
   415  
   416  pcard->dma_base_resource.start = dma_bar_phys_addr;

[ snip ]

   509  dev_dbg(>pdev->dev, "%s() complete!\n", __func__);
   510  mutex_unlock(>sem);
   511  return 0;
   512  
   513  err_remove_sysfs:
   514  sysfs_remove_files(>dev.kobj, kp_attr_list);
   515  err_free_irq:
   516  free_irq(pcard->pdev->irq, pcard);
   517  err_disable_msi:
   518  pci_disable_msi(pcard->pdev);
   519  err_unmap_dma:
   520  iounmap(pcard->dma_bar_base);
   521  pci_release_region(pdev, DMA_BAR);
   522  pcard->dma_bar_base = NULL;
   523  err_unmap_regs:
   524  iounmap(pcard->regs_bar_base);
   525  pci_release_region(pdev, REG_BAR);
   526  pcard->regs_bar_base = NULL;

err_release_dma:
pci_release_region(pdev, DMA_BAR);
err_unmap_dma:
iounmap(pcard->dma_bar_base);
err_release_reg:
pci_release_region(pdev, REG_BAR);
err_unmap_reg:
iounmap(pcard->regs_bar_base);

I moved swapped the pci_release_region() and the iounmap() order.  There
is no need to set "pcard->regs_bar_base = NULL;" so just remove that.

   527  err_disable_device:
   528  pci_disable_device(pcard->pdev);
   529  err_remove_ida:
   530  mutex_unlock(>sem);
   531  ida_simple_remove(_num_ida, pcard->card_num);
   532  err_free_pcard

Re: [PATCH 1/2] staging: rtl8712: r8712_setdatarate_cmd(): Change

[Dan Carpenter]

On Mon, Jun 10, 2019 at 10:02:27AM +0530, Nishka Dasgupta wrote:
> On 07/06/19 7:45 PM, Dan Carpenter wrote:
> > Probably you sent this patch unintentionally.  The subject doesn't make
> > any sort of sense.  :P
> 
> So the problem with the subject line is that git send-email and vim (as
> configured on my laptop) tend to line-wrap even the subject line. Since I
> have two patches that do the same thing for different functions, I felt I
> should have the driver and the function name in the subject line (to avoid
> confusion between the patches and to allow for easy searching later). But
> that doesn't leave enough space in the subject line for "Change return
> values/type" or any other descriptive message. What should I do?
> 

I don't really care.

[PATCH] staging: rtl8712: clean up r8712_setdatarate_cmd() return type

regards,
dan carpenter

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

Re: [PATCH 2/2] staging: rtl8712: r8712_createbss_cmd(): Change

[Dan Carpenter]

Fix the subject.

On Fri, Jun 07, 2019 at 07:36:58PM +0530, Nishka Dasgupta wrote:
> Change return values of r8712_createbss_cmd from _SUCCESS and _FAIL to 0
> and -ENOMEM respectively.
> Change return type of the function from unsigned to int to reflect this.
> Change call site to check for 0 instead of _SUCCESS.
> (Instead of !=0, simply passing the function output to the conditional
  ^^
> will do.)
  ^

Remove this line.

Otherwise it looks ok.  Please resend.

regards,
dan carpenter

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

Re: [PATCH 1/2] staging: rtl8712: r8712_setdatarate_cmd(): Change

[Dan Carpenter]

Probably you sent this patch unintentionally.  The subject doesn't make
any sort of sense.  :P

On Fri, Jun 07, 2019 at 07:36:57PM +0530, Nishka Dasgupta wrote:
> Change the return values of function r8712_setdatarate_cmd from _SUCCESS
> and _FAIL to 0 and -ENOMEM respectively.
> Change the return type of the function from u8 to int to reflect this.
> Change the call site of the function to check for 0 instead of _SUCCESS.
> (Checking that the return value != 0 is not necessary; the return value
  ^^^
> itself can simply be passed into the conditional.)
  ^

This is obvious.  No need to mention it in the commit message.

> diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c 
> b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> index b424b8436fcf..761e2ba68a42 100644
> --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> @@ -1367,7 +1367,7 @@ static int r8711_wx_set_rate(struct net_device *dev,
>   datarates[i] = 0xff;
>   }
>   }
> - if (r8712_setdatarate_cmd(padapter, datarates) != _SUCCESS)
> + if (r8712_setdatarate_cmd(padapter, datarates))
>   ret = -ENOMEM;
>
>   return ret;


It would be better to write this like so:

ret = r8712_setdatarate_cmd(padapter, datarates);
if (ret)
return ret;

return 0;

Or you could write it like:

return r8712_setdatarate_cmd(padapter, datarates);

Which ever one you prefer is fine.

regards,
dan carpenter

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

Re: [PATCH 3/7] media: cedrus: Fix decoding for some H264 videos

[Dan Carpenter]

On Mon, Jun 03, 2019 at 05:37:17PM +0200, Jernej Škrabec wrote:
> Dne ponedeljek, 03. junij 2019 ob 13:55:36 CEST je Maxime Ripard napisal(a):
> > int current = 0;
> > 
> > while (current < num) {
> > int tmp = min(num - current, 32);
> > 
> > cedrus_write(dev, VE_H264_TRIGGER_TYPE, 0x3 | (current << 8))
   ^^^
This should be "tmp << 8" instead of "current << 8" though.


> > while (...)
> >...
> > 
> > current += tmp;
> > }
> 

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

Re: [PATCH 7/7] staging: kpc2000: fix incorrect code comment in core.c

[Dan Carpenter]

On Tue, Jun 04, 2019 at 12:29:16AM +0200, Simon Sandström wrote:
> Step 11 was removed from kp2000_pcie_probe in a previous commit but the
> comment was not changed to reflect this, so do it now.
> 
> Signed-off-by: Simon Sandström 
> ---
>  drivers/staging/kpc2000/kpc2000/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/kpc2000/kpc2000/core.c 
> b/drivers/staging/kpc2000/kpc2000/core.c
> index 2d8d188624f7..cd3876f1ce17 100644
> --- a/drivers/staging/kpc2000/kpc2000/core.c
> +++ b/drivers/staging/kpc2000/kpc2000/core.c
> @@ -501,7 +501,7 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
>   goto out10;
>  
>   /*
> -  * Step 12: Enable IRQs in HW
> +  * Step 11: Enable IRQs in HW

I don't have a problem with this patch but for the future these numbers
don't add any value.  And the numbered out labels are sort of ugly.  The
label name should say what the label does just like a function name says
what the function does.  Really a lot of these comments in the probe
function are very obvious and don't add information (delete them).


   491  /*
   492   * Step 9: Setup sysfs attributes
   493   */
   494  err = sysfs_create_files(&(pdev->dev.kobj), kp_attr_list);

The comment is probably less informative than the code.

   495  if (err) {
   496  dev_err(>dev, "Failed to add sysfs files: %d\n", 
err);
   497  goto out9;

What does goto out9 do?

   498  }
   499  
   500  /*
   501   * Step 10: Probe cores
   502   */
   503  err = kp2000_probe_cores(pcard);
   504  if (err)
   505  goto out10;

Hopefully, goto out10 deletes the sysfs files but we don't know because
the label doesn't give any clues away.  We have to search for it and
then come back.

   506  
   507  /*
   508   * Step 12: Enable IRQs in HW
   509   */
   510  writel(KPC_DMA_CARD_IRQ_ENABLE | 
KPC_DMA_CARD_USER_INTERRUPT_MODE,
   511 pcard->dma_common_regs);
   512  
   513  dev_dbg(>pdev->dev, "kp2000_pcie_probe() complete!\n");
   514  mutex_unlock(>sem);
   515  return 0;
   516  
   517  out10:

err_remove_sysfs:

   518  sysfs_remove_files(&(pdev->dev.kobj), kp_attr_list);
   519  out9:

err_free_irq:

   520  free_irq(pcard->pdev->irq, pcard);
   521  out8b:

err_disable_msi:

   522  pci_disable_msi(pcard->pdev);
   523  out8a:
   524  out7:
   525  out6:

err_unmap_dma:

   526  iounmap(pcard->dma_bar_base);
   527  pci_release_region(pdev, DMA_BAR);
   528  pcard->dma_bar_base = NULL;
   529  out5:

err_unmap_regs:

   530  iounmap(pcard->regs_bar_base);
   531  pci_release_region(pdev, REG_BAR);
   532  pcard->regs_bar_base = NULL;

Something like that is way more useful because then you don't have to
scroll back and forth because new the label names are useful.

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

Re: [PATCH] Staging: emxx_udc: fix warning "sum of probable bitmasks, consider |"

[Dan Carpenter]

On Wed, Jun 05, 2019 at 12:04:43PM +0530, Hariprasad Kelam wrote:
> On Mon, Jun 03, 2019 at 09:04:57PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Jun 04, 2019 at 12:24:12AM +0530, Hariprasad Kelam wrote:
> > > Knowing the fact that operator '|' is faster than '+'.
> > > Its better we replace + with | in this case.
> > > 
> > > Issue reported by coccicheck
> > > drivers/staging/emxx_udc/emxx_udc.h:94:34-35: WARNING: sum of probable
> > > bitmasks, consider |
> > > 
> > > Signed-off-by: Hariprasad Kelam 
> > > ---
> > >  drivers/staging/emxx_udc/emxx_udc.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/staging/emxx_udc/emxx_udc.h 
> > > b/drivers/staging/emxx_udc/emxx_udc.h
> > > index b8c3dee..88d6bda 100644
> > > --- a/drivers/staging/emxx_udc/emxx_udc.h
> > > +++ b/drivers/staging/emxx_udc/emxx_udc.h
> > > @@ -91,7 +91,7 @@ int vbus_irq;
> > >  #define BIT300x4000
> > >  #define BIT310x8000
> > 
> > All of those BITXX defines should be removed and the "real" BIT(X) macro
> > used instead.
> Yes will send separate patch  to address this.
> > 
> > > -#define TEST_FORCE_ENABLE(BIT18 + BIT16)
> > > +#define TEST_FORCE_ENABLE(BIT18 | BIT16)
> > 
> > It really doesn't matter, a good compiler will have already turned this
> > into a constant value so you really do not know if this is less/faster
> > code or not, right?
> > 
> > Did you look at the output to verify this actually changed anything?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Ok . Treating this as false postive from coccicheck.

I liked the patch.  | is way more normal and readable than +.  It's just
the commit message was bogus.

I would be very surprised if this coccicheck found anything that wasn't
a compile time constant like this.

regards,
dan carpenter

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

Re: [PATCH] staging: comedi: Remove variable runflags

[Dan Carpenter]

Anyway, Greg was never going to apply this so it's not worth worrying
about too much.

regards,
dan carpenter

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

Re: [PATCH] staging: kpc2000: replace bogus variable name in core.c

[Dan Carpenter]

On Wed, May 29, 2019 at 09:42:22PM +0200, Simon Sandström wrote:
> "struct kp2000_regs temp" has nothing to do with temperatures, so
> replace it with the more proper name "regs".
> 
> Signed-off-by: Simon Sandström 
> ---

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: ieee80211.c: Remove leading p from variable names

[Dan Carpenter]

On Fri, May 31, 2019 at 02:33:59AM +0530, Nishka Dasgupta wrote:
> Remove leading p from the names of the following pointer variables:
> - pregistrypriv
> - pdev_network
> - pie
> - pbuf.
> Issue found with Coccinelle.
> 
> Signed-off-by: Nishka Dasgupta 

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 function r8712_init_mlme_priv

[Dan Carpenter]

On Fri, May 31, 2019 at 02:25:31AM +0530, Nishka Dasgupta wrote:
> Delete r8712_init_mlme_priv as it does nothing except call
> _init_mlme_priv, and rename _init_mlme_priv to
> r8712_init_mlme_priv.
> Change the type of the new r8712_init_mlme_priv (formerly _init_mlme_priv)
> to (non-static) int, from static sint.
> 
> Signed-off-by: Nishka Dasgupta 
> ---

Reviewed-by: Dan Carpenter 

Looks good, thanks.

regards,
dan carpenter

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

Re: [PATCH 3/4] staging: kpc2000: add missing spaces in core.c

[Dan Carpenter]

On Wed, May 29, 2019 at 05:54:19PM +0200, Simon Sandström wrote:
> On Mon, May 27, 2019 at 10:31:59AM +0300, Dan Carpenter wrote:
> > On Fri, May 24, 2019 at 01:08:01PM +0200, Simon Sandström wrote:
> > > [..]
> > > - ret = copy_to_user((void*)ioctl_param, (void*), 
> > > sizeof(temp));
> > > + ret = copy_to_user((void *)ioctl_param, (void *), 
> > > sizeof(temp));
> > >   if (ret)
> > >   return -EFAULT;
> > 
> > This should really be written like so:
> > 
> > if (copy_to_user((void __user *)ioctl_param, ,
> >  sizeof(temp)))
> > return -EFAULT;
> > 
> > temp is really the wrong name.  "temp" is for temperatures.  "tmp" means
> > temporary.  But also "tmp" is wrong here because it's not a temporary
> > variable.  It's better to call it "regs" here.
> > 
> > regards,
> > dan carpenter
> > 
> 
> I agree, but I don't think it fits within this patch. I can send a
> separate patch with this change.

You could send the other chunk as a separate patch, but I don't think it
makes sense to apply this chunk when really it just needs to be
re-written.

I normally don't complain too much about mechanical no-thought patches,
but in this case the function is very sub-par and should be re-written.

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

Re: [PATCH v2 2/2] staging: erofs: fix i_blocks calculation

[Dan Carpenter]

On Tue, May 28, 2019 at 11:02:12AM +0800, Chao Yu wrote:
> On 2019/5/28 10:36, Gao Xiang wrote:
> > For compressed files, i_blocks should not be calculated
> > by using i_size. i_u.compressed_blocks is used instead.
> > 
> > In addition, i_blocks was miscalculated for non-compressed
> > files previously, fix it as well.
> > 
> > Signed-off-by: Gao Xiang 
> > ---
> > change log v2:
> >  - fix description in commit message
> >  - fix to 'inode->i_blocks = nblks << LOG_SECTORS_PER_BLOCK'
> > 
> > Thanks,
> > Gao Xiang
> > 
> >  drivers/staging/erofs/inode.c | 14 --
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/erofs/inode.c b/drivers/staging/erofs/inode.c
> > index 8da144943ed6..6e67e018784e 100644
> > --- a/drivers/staging/erofs/inode.c
> > +++ b/drivers/staging/erofs/inode.c
> > @@ -20,6 +20,7 @@ static int read_inode(struct inode *inode, void *data)
> > struct erofs_vnode *vi = EROFS_V(inode);
> > struct erofs_inode_v1 *v1 = data;
> > const unsigned int advise = le16_to_cpu(v1->i_advise);
> > +   erofs_blk_t nblks = 0;
> >  
> > vi->data_mapping_mode = __inode_data_mapping(advise);
> >  
> > @@ -60,6 +61,10 @@ static int read_inode(struct inode *inode, void *data)
> > le32_to_cpu(v2->i_ctime_nsec);
> >  
> > inode->i_size = le64_to_cpu(v2->i_size);
> > +
> > +   /* total blocks for compressed files */
> > +   if (vi->data_mapping_mode == EROFS_INODE_LAYOUT_COMPRESSION)
> > +       nblks = v2->i_u.compressed_blocks;
> 
> Xiang,
> 
> It needs to use le32_to_cpu(). ;)
> 

I wonder it the kbuild bot is going to send an email about that...
Hopefully these sorts of bugs get detected with Sparse CF=-D__CHECK_ENDIAN__

regards,
dan carpenter

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

Re: [PATCH 3/4] staging: kpc2000: add missing spaces in core.c

[Dan Carpenter]

On Fri, May 24, 2019 at 01:08:01PM +0200, Simon Sandström wrote:
> Fixes checkpatch.pl errors "space required before the open brace '{'"
> and "(foo*)" should be "(foo *)".
> 
> Signed-off-by: Simon Sandström 
> ---
>  drivers/staging/kpc2000/kpc2000/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/kpc2000/kpc2000/core.c 
> b/drivers/staging/kpc2000/kpc2000/core.c
> index 40f65f96986b..bb3b427a72b1 100644
> --- a/drivers/staging/kpc2000/kpc2000/core.c
> +++ b/drivers/staging/kpc2000/kpc2000/core.c
> @@ -308,7 +308,7 @@ static long kp2000_cdev_ioctl(struct file *filp, unsigned 
> int ioctl_num,
>  
>   dev_dbg(>pdev->dev, "kp2000_cdev_ioctl(filp = [%p], ioctl_num = 
> 0x%08x, ioctl_param = 0x%016lx) pcard = [%p]\n", filp, ioctl_num, 
> ioctl_param, pcard);
>  
> - switch (ioctl_num){
> + switch (ioctl_num) {
>   case KP2000_IOCTL_GET_CPLD_REG: return 
> readq(pcard->sysinfo_regs_base + REG_CPLD_CONFIG);
>   case KP2000_IOCTL_GET_PCIE_ERROR_REG:   return 
> readq(pcard->sysinfo_regs_base + REG_PCIE_ERROR_COUNT);
>  
> @@ -326,7 +326,7 @@ static long kp2000_cdev_ioctl(struct file *filp, unsigned 
> int ioctl_num,
>   temp.ddna = pcard->ddna;
>   temp.cpld_reg = readq(pcard->sysinfo_regs_base + 
> REG_CPLD_CONFIG);
>  
> - ret = copy_to_user((void*)ioctl_param, (void*), 
> sizeof(temp));
> + ret = copy_to_user((void *)ioctl_param, (void *), 
> sizeof(temp));
>   if (ret)
>   return -EFAULT;

This should really be written like so:

if (copy_to_user((void __user *)ioctl_param, ,
 sizeof(temp)))
return -EFAULT;

temp is really the wrong name.  "temp" is for temperatures.  "tmp" means
temporary.  But also "tmp" is wrong here because it's not a temporary
variable.  It's better to call it "regs" here.

regards,
dan carpenter

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

Re: [PATCH] staging: kpc2000: simplify nested conditionals that just return a boolean.

[Dan Carpenter]

On Fri, May 24, 2019 at 01:19:26PM +0100, Jeremy Sowden wrote:
> kp2000_check_uio_irq contained a pair of nested ifs which each evaluated
> a bitwise operation.  If both operations yielded true, the function
> returned 1; otherwise it returned 0.
> 
> Replaced the whole thing with one return statement that evaluates the
> combination of both bitwise operations.
> 
> Signed-off-by: Jeremy Sowden 
> ---
> This applies to staging-testing.
> 
> I was in two minds whether to send this patch or something less terse:
> 
> + return (interrupt_active & irq_check_mask) &&
> +(interrupt_mask_inv & irq_check_mask);

Yeah.  I would prefer this.

regards,
dan carpenter


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

Re: [PATCH] staging: ks7010: Remove initialisation

[Dan Carpenter]

On Fri, May 24, 2019 at 11:22:38AM +0530, Nishka Dasgupta wrote:
> The initial value of return variable ret is never used, so it can be
> removed.
> Issue found with Coccinelle.
> 
> Signed-off-by: Nishka Dasgupta 
> ---
>  drivers/staging/ks7010/ks_hostif.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/ks7010/ks_hostif.c 
> b/drivers/staging/ks7010/ks_hostif.c
> index e089366ed02a..3775fd4b89ae 100644
> --- a/drivers/staging/ks7010/ks_hostif.c
> +++ b/drivers/staging/ks7010/ks_hostif.c
> @@ -1067,7 +1067,7 @@ int hostif_data_request(struct ks_wlan_private *priv, 
> struct sk_buff *skb)
>   unsigned int length = 0;
>   struct hostif_data_request *pp;
>   unsigned char *p;
> - int result = 0;
> + int result;

You should get rid of the result variable completely and just use "ret"
instead.  No need for two variables.

regards,
dan carpenter

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

Re: [PATCH] staging/gasket: Fix string split

[Dan Carpenter]

On Thu, May 23, 2019 at 05:11:56PM +0200, 李天正 wrote:
> Hello，
> we are doing a project in the university and we cooperated to make this
> patch. Some warnings are found by Mr.Zhang.

Use the Reported-by to show who found the bug or Co-developed-by: if you
both wrote code.

regards,
dan carpenter

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

Re: [PATCH] staging: fieldbus: anybuss: Remove unnecessary variables

[Dan Carpenter]

On Thu, May 23, 2019 at 09:27:02AM +0100, Jeremy Sowden wrote:
> On 2019-05-23, at 13:51:18 +0530, Nishka Dasgupta wrote:
> > On 23/05/19 12:52 PM, Greg KH wrote:
> > > On Thu, May 23, 2019 at 12:05:01PM +0530, Nishka Dasgupta wrote:
> > > Also, you forgot to cc: Sven on this patch, please always use the output
> > > of scripts/get_maintainer.pl.
> >
> > Which arguments should I use? If I use --nokeywords, --nogit,
> > --nogit-fallback and --norolestats then only your name and the two
> > mailing lists show up.  (Also, regarding the mailing lists: every mail
> > sent to linux-ker...@vger.kernel.org is bouncing; should I not send to
> > that list anymore?)
> 
> He is listed in the TODO:
> 
>   $ cat drivers/staging/fieldbus/TODO
>   TODO:
>   -Get more people/drivers to use the Fieldbus userspace ABI. It requires
>verification/sign-off by multiple users.
> 
>   Contact: Sven Van Asbroeck 

Sven, you should add yourself to the MAINTAINERS file.

regards,
dan carpenter


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

Re: [PATCH] staging: rtl8723bs: Add missing blank lines

[Dan Carpenter]

On Tue, May 21, 2019 at 09:46:55PM -0300, Fabio Lima wrote:
> This patch resolves the following warning from checkpatch.pl
> WARNING: Missing a blank line after declarations
> 
> Signed-off-by: Fabio Lima 
> ---
>  drivers/staging/rtl8723bs/core/rtw_debug.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_debug.c 
> b/drivers/staging/rtl8723bs/core/rtw_debug.c
> index 9f8446ccf..853362381 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_debug.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_debug.c
> @@ -382,6 +382,7 @@ ssize_t proc_set_roam_tgt_addr(struct file *file, const 
> char __user *buffer, siz
>   if (buffer && !copy_from_user(tmp, buffer, sizeof(tmp))) {
>  
>   int num = sscanf(tmp, "%hhx:%hhx:%hhx:%hhx:%hhx:%hhx", addr, 
> addr+1, addr+2, addr+3, addr+4, addr+5);
> +
>   if (num == 6)
>   memcpy(adapter->mlmepriv.roam_tgt_addr, addr, ETH_ALEN);
>  

I'm sorry but this function is really such nonsense.  Can you send a
patch to re-write it instead?

drivers/staging/rtl8723bs/core/rtw_debug.c
   371  ssize_t proc_set_roam_tgt_addr(struct file *file, const char __user 
*buffer, size_t count, loff_t *pos, void *data)
   372  {
   373  struct net_device *dev = data;
   374  struct adapter *adapter = (struct adapter 
*)rtw_netdev_priv(dev);
   375  
   376  char tmp[32];
   377  u8 addr[ETH_ALEN];
   378  
   379  if (count < 1)

This check is silly.  I guess the safest thing is to change it to:
if (count < sizeof(tmp))

   380  return -EFAULT;

It should be return -EINVAL;

   381  
   382  if (buffer && !copy_from_user(tmp, buffer, sizeof(tmp))) {

Remove the check for if the user passes a NULL buffer, because that's
already handled in copy_from_user().  Return -EFAULT if copy_from_user()
fails.

if (copy_from_user(tmp, buffer, sizeof(tmp)))
return -EFAULT;


   383  

Extra blank line.

   384  int num = sscanf(tmp, "%hhx:%hhx:%hhx:%hhx:%hhx:%hhx", 
addr, addr+1, addr+2, addr+3, addr+4, addr+5);

You will need to move the num declaration to the start of the function.

   385  if (num == 6)
   386  memcpy(adapter->mlmepriv.roam_tgt_addr, addr, 
ETH_ALEN);

If num != 6 then return -EINVAL;

   387  
   388  DBG_871X("set roam_tgt_addr to "MAC_FMT"\n", 
MAC_ARG(adapter->mlmepriv.roam_tgt_addr));
   389  }
   390  
   391  return count;
   392  }

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

Re: [PATCH] staging: rtl8192u: Remove an unnecessary NULL check

[Dan Carpenter]

On Tue, May 21, 2019 at 03:57:46PM -0700, Nick Desaulniers wrote:
> > > -   if (param->u.wpa_ie.len > MAX_WPA_IE_LEN ||
> > > -   (param->u.wpa_ie.len && !param->u.wpa_ie.data))
> >
> > Right so, the types in this expression:
> >
> > param: struct ieee_param*
> > param->u: *anonymous union*
> > param->u.wpa_ie: *anonymous struct*
> > param->u.wpa_ie.len: u32
> > param->u.wpa_ie.data: u8 [0]
> > as defined in drivers/staging/rtl8192u/ieee80211/ieee80211.h#L295
> > https://github.com/ClangBuiltLinux/linux/blob/9c7db5004280767566e91a33445bf93aa479ef02/drivers/staging/rtl8192u/ieee80211/ieee80211.h#L295-L322
> >
> > so this is a tricky case, because in general array members can never
> > themselves be NULL,


Unless they array was the first struct member, obviously.


> >  and usually I trust -Wpointer-bool-conversion, but
> > this is a special case because of the flexible array member:

Nah.  It's the same thing.  That patch is fine.

regards,
dan carpenter

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

Re: [PATCH] staging: rtl8192u: Remove an unnecessary NULL check

[Dan Carpenter]

On Tue, May 21, 2019 at 10:42:21AM -0700, Nathan Chancellor wrote:
> Clang warns:
> 
> drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:2663:47: warning:
> address of array 'param->u.wpa_ie.data' will always evaluate to 'true'
> [-Wpointer-bool-conversion]
> (param->u.wpa_ie.len && !param->u.wpa_ie.data))
> ~^~~~
> 
> This was exposed by commit deabe03523a7 ("Staging: rtl8192u: ieee80211:
> Use !x in place of NULL comparisons") because we disable the warning
> that would have pointed out the comparison against NULL is also false:
> 

Heh.  Weird.  Why would people disable one and not the other?

regards,
dan carpenter

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

Re: [PATCH] staging/gasket: Fix string split

[Dan Carpenter]

On Tue, May 21, 2019 at 05:07:28PM +0200, Tianzheng Li wrote:
> This patch removes unnecessary quoted string splits.
> 
> Signed-off-by: Tianzheng Li 
> Signed-off-by: Jie Zhang 

What do the two sign off mean here?  What did Jie Zhang do?  Who wrote
this patch?

Co-developed-by?  Reviewed-by?

regards,
dan carpenter

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

Re: [PATCH] staging: fieldbus: anybuss: force address space conversion

[Dan Carpenter]

On Tue, May 21, 2019 at 10:51:16AM -0400, Sven Van Asbroeck wrote:
> The regmap's context (stored as 'void *') consists of a pointer to
> __iomem. Mixing __iomem and non-__iomem addresses generates
> sparse warnings.
> 
> Fix by using __force when converting to/from 'void __iomem *' and
> the regmap's context.
> 
> Signed-off-by: Sven Van Asbroeck 
> ---
>  drivers/staging/fieldbus/anybuss/arcx-anybus.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/fieldbus/anybuss/arcx-anybus.c 
> b/drivers/staging/fieldbus/anybuss/arcx-anybus.c
> index a167fb68e355..8126e5535ada 100644
> --- a/drivers/staging/fieldbus/anybuss/arcx-anybus.c
> +++ b/drivers/staging/fieldbus/anybuss/arcx-anybus.c
> @@ -114,7 +114,7 @@ static void export_reset_1(struct device *dev, bool 
> assert)
>  static int read_reg_bus(void *context, unsigned int reg,
>   unsigned int *val)
>  {
> - void __iomem *base = context;
> + void __iomem *base = (__force void __iomem *)context;


There is no need to use __force.  Just:

void __iomem *base = (void __iomem *)context;

For the rest as well.

regards,
dan carpenter

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

Re: [PATCH v3] staging: fieldbus: core: fix ->poll() annotation

[Dan Carpenter]

On Tue, May 21, 2019 at 10:20:09AM -0400, Sven Van Asbroeck wrote:
> From: Oscar Gomez Fuente 
> 
> ->poll() functions should return __poll_t, but the fieldbus
> core's poll() does not. This generates a sparse warning.
> 
> Fix the ->poll() return value, and use recommended __poll_t
> constants (EPOLLxxx).
> 
> Signed-off-by: Oscar Gomez Fuente 
> ---

If you're resending someone's patch, you have to add your own Signed off
by line as well.  Everyone who touches a patch has to sign that they
didn't add any of SCO's all powerful UnixWare source code into the
patch.

regards,
dan carpenter


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

Re: [PATCH v3 3/6] staging: kpc2000: simplified kp2000_device retrieval in device attributes call-backs.

[Dan Carpenter]

On Fri, May 17, 2019 at 01:54:51PM +0200, Greg KH wrote:
> On Fri, May 17, 2019 at 12:03:12PM +0100, Jeremy Sowden wrote:
> >  static ssize_t  show_attr(struct device *dev, struct device_attribute 
> > *attr, char *buf)
> >  {
> > -struct pci_dev *pdev = to_pci_dev(dev);
> > -struct kp2000_device *pcard;
> > -
> > -if (!pdev)  return -ENXIO;
> > -pcard = pci_get_drvdata(pdev);
> > -if (!pcard)  return -ENXIO;
> > +struct kp2000_device *pcard = dev_get_drvdata(dev);
> 
> Wait, dev_get_drvdata() is not returning you the same pointer that
> pci_get_drvdata() does.  So I think this is now broken :(
> 

It looks sort of weird but it's fine.

> What this should look like is this:
>   struct pci_dev *pdev = to_pci_dev(dev);
>   struct kp200_device *pcard = pci_get_drvdata(pdev);
> 
>   if (!pcard)
>   return -ENODEV;
> 
> that is IF the driver really is setting the pci dev data to NULL when
> the device is removed from the driver.  Is it?

Yes.  The pci_get_drvdata() is only set to NULL after we remove the
sysfs files so pci_get_drvdata() always returns a valid pointer.

regards,
dan carpenter

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

Re: [PATCH] staging: rtl8723bs: core: rtw_recv: fix warning Comparison to NULL

[Dan Carpenter]

On Wed, May 15, 2019 at 11:15:36PM +0530, Hariprasad Kelam wrote:
> @@ -1042,7 +1042,7 @@ sint sta2ap_data_frame(
>   }
>  
>   *psta = rtw_get_stainfo(pstapriv, pattrib->src);
> - if (*psta == NULL) {
> + if (!*psta == NULL) {
^^
It's surprising that this didn't cause some kind of warning somewhere...

>   RT_TRACE(_module_rtl871x_recv_c_, _drv_err_, ("can't 
> get psta under AP_MODE; drop pkt\n"));
>   DBG_871X("issue_deauth to sta =" MAC_FMT " for the 
> reason(7)\n", MAC_ARG(pattrib->src));
>  

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

Re: [PATCH v2 0/5] staging: kpc2000: assorted fixes

[Dan Carpenter]

Thanks!

Reviewed-by: Dan Carpenter 

regards,
dan carpenter

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

Re: [PATCH 3/5] staging: kpc2000: added designated initializers to two structs.

[Dan Carpenter]

On Wed, May 15, 2019 at 02:09:49PM +0300, Dan Carpenter wrote:
> On Wed, May 15, 2019 at 11:34:52AM +0100, Jeremy Sowden wrote:
> > Fixed the following two sparse warnings by using designated
> > initializers:
> > 
> >   drivers/staging/kpc2000/kpc2000/cell_probe.c:101:34: warning: Using plain 
> > integer as NULL pointer
> >   drivers/staging/kpc2000/kpc2000/cell_probe.c:364:34: warning: Using plain 
> > integer as NULL pointer
> > 
> > Signed-off-by: Jeremy Sowden 
> > ---
> >  drivers/staging/kpc2000/kpc2000/cell_probe.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/kpc2000/kpc2000/cell_probe.c 
> > b/drivers/staging/kpc2000/kpc2000/cell_probe.c
> > index 30e6f176ddfa..9cb745f4323a 100644
> > --- a/drivers/staging/kpc2000/kpc2000/cell_probe.c
> > +++ b/drivers/staging/kpc2000/kpc2000/cell_probe.c
> > @@ -94,7 +94,7 @@ void parse_core_table_entry(struct core_table_entry *cte, 
> > const u64 read_val, co
> >  static int probe_core_basic(unsigned int core_num, struct kp2000_device 
> > *pcard,
> > char *name, const struct core_table_entry cte)
> >  {
> > -struct mfd_cell  cell = {0};
> > +struct mfd_cell  cell = { .id = core_num, .name = name };
> >  struct resource  resources[2];
> >  
> >  struct kpc_core_device_platdata  core_pdata = {
> > @@ -315,7 +315,7 @@ static int probe_core_uio(unsigned int core_num, struct 
> > kp2000_device *pcard,
> >  
> >  static int  create_dma_engine_core(struct kp2000_device *pcard, size_t 
> > engine_regs_offset, int engine_num, int irq_num)
> >  {
> > -struct mfd_cell  cell = {0};
> > +struct mfd_cell  cell = { .id = engine_num };
> >  struct resource  resources[2];
> >  
> 
> These changes make no sense because we just write over it later.
> 
> Maybe you're going to fix it up later in the patch series, perhaps but
> that's not how it's done.  Each patch should do "one thing", not "half
> and thing and then half a thing later in the series possibly (I am
> reviewing the patches in order so I don't know)".
> 

I've finished reviewing the series and we never complete the other half
of this patch.

regards,
dan carpenter

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

Re: [PATCH 3/5] staging: kpc2000: added designated initializers to two structs.

[Dan Carpenter]

On Wed, May 15, 2019 at 11:34:52AM +0100, Jeremy Sowden wrote:
> Fixed the following two sparse warnings by using designated
> initializers:
> 
>   drivers/staging/kpc2000/kpc2000/cell_probe.c:101:34: warning: Using plain 
> integer as NULL pointer
>   drivers/staging/kpc2000/kpc2000/cell_probe.c:364:34: warning: Using plain 
> integer as NULL pointer
> 
> Signed-off-by: Jeremy Sowden 
> ---
>  drivers/staging/kpc2000/kpc2000/cell_probe.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/kpc2000/kpc2000/cell_probe.c 
> b/drivers/staging/kpc2000/kpc2000/cell_probe.c
> index 30e6f176ddfa..9cb745f4323a 100644
> --- a/drivers/staging/kpc2000/kpc2000/cell_probe.c
> +++ b/drivers/staging/kpc2000/kpc2000/cell_probe.c
> @@ -94,7 +94,7 @@ void parse_core_table_entry(struct core_table_entry *cte, 
> const u64 read_val, co
>  static int probe_core_basic(unsigned int core_num, struct kp2000_device 
> *pcard,
>   char *name, const struct core_table_entry cte)
>  {
> -struct mfd_cell  cell = {0};
> +struct mfd_cell  cell = { .id = core_num, .name = name };
>  struct resource  resources[2];
>  
>  struct kpc_core_device_platdata  core_pdata = {
> @@ -315,7 +315,7 @@ static int probe_core_uio(unsigned int core_num, struct 
> kp2000_device *pcard,
>  
>  static int  create_dma_engine_core(struct kp2000_device *pcard, size_t 
> engine_regs_offset, int engine_num, int irq_num)
>  {
> -struct mfd_cell  cell = {0};
> +struct mfd_cell  cell = { .id = engine_num };
>  struct resource  resources[2];
>  

These changes make no sense because we just write over it later.

Maybe you're going to fix it up later in the patch series, perhaps but
that's not how it's done.  Each patch should do "one thing", not "half
and thing and then half a thing later in the series possibly (I am
reviewing the patches in order so I don't know)".

regards,
dan carpenter

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

[PATCH] staging: kpc2000: double unlock in error handling in kpc_dma_transfer()

[Dan Carpenter]

The goto err_descr_too_many; calls unlock_engine() so this unlocks
twice.

Fixes: 7df95299b94a ("staging: kpc2000: Add DMA driver")
Signed-off-by: Dan Carpenter 
---
 drivers/staging/kpc2000/kpc_dma/fileops.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c 
b/drivers/staging/kpc2000/kpc_dma/fileops.c
index 5741d2b49a7d..d96eaa5e72a6 100644
--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
@@ -116,13 +116,11 @@ int  kpc_dma_transfer(struct dev_private_data *priv, 
struct kiocb *kcb, unsigned
if (desc_needed >= ldev->desc_pool_cnt){
dev_warn(>ldev->pldev->dev, "mapped_entry_count = %d  
  num_descrs_needed = %dnum_descrs_avail = %dTOO MANY to ever 
complete!\n", acd->mapped_entry_count, desc_needed, num_descrs_avail);
rv = -EAGAIN;
-   unlock_engine(ldev);
goto err_descr_too_many;
}
if (desc_needed > num_descrs_avail){
dev_warn(>ldev->pldev->dev, "mapped_entry_count = %d  
  num_descrs_needed = %dnum_descrs_avail = %dToo many to complete right 
now.\n", acd->mapped_entry_count, desc_needed, num_descrs_avail);
rv = -EMSGSIZE;
-   unlock_engine(ldev);
goto err_descr_too_many;
}
 
-- 
2.20.1

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

[PATCH] staging: wilc1000: Fix some double unlock bugs in wilc_wlan_cleanup()

[Dan Carpenter]

If ->hif_read_reg() or ->hif_write_reg() fail then the code unlocks
and keeps executing.  It should just return.

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

diff --git a/drivers/staging/wilc1000/wilc_wlan.c 
b/drivers/staging/wilc1000/wilc_wlan.c
index 0a713409ea98..95eaf8fdf4f2 100644
--- a/drivers/staging/wilc1000/wilc_wlan.c
+++ b/drivers/staging/wilc1000/wilc_wlan.c
@@ -1076,13 +1076,17 @@ void wilc_wlan_cleanup(struct net_device *dev)
acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
 
ret = wilc->hif_func->hif_read_reg(wilc, WILC_GP_REG_0, );
-   if (!ret)
+   if (!ret) {
release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
+   return;
+   }
 
ret = wilc->hif_func->hif_write_reg(wilc, WILC_GP_REG_0,
(reg | ABORT_INT));
-   if (!ret)
+   if (!ret) {
release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
+   return;
+   }
 
release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
wilc->hif_func->hif_deinit(NULL);
-- 
2.20.1

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

[PATCH] staging: vc04_services: prevent integer overflow in create_pagelist()

[Dan Carpenter]

The create_pagelist() "count" parameter comes from the user in
vchiq_ioctl() and it could overflow.  If you look at how create_page()
is called in vchiq_prepare_bulk_data(), then the "size" variable is an
int so it doesn't make sense to allow negatives or larger than INT_MAX.

I don't know this code terribly well, but I believe that typical values
of "count" are typically quite low and I don't think this check will
affect normal valid uses at all.

The "pagelist_size" calculation can also overflow on 32 bit systems, but
not on 64 bit systems.  I have added an integer overflow check for that
as well.

The Raspberry PI doesn't offer the same level of memory protection that
x86 does so these sorts of bugs are probably not super critical to fix.

Fixes: 71bad7f08641 ("staging: add bcm2708 vchiq driver")
Signed-off-by: Dan Carpenter 
---
 .../vc04_services/interface/vchiq_arm/vchiq_2835_arm.c   | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c 
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index a9a22917ecdb..a5b5840ff91a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -368,9 +368,18 @@ create_pagelist(char __user *buf, size_t count, unsigned 
short type)
int dma_buffers;
dma_addr_t dma_addr;
 
+   if (count >= INT_MAX - PAGE_SIZE)
+   return NULL;
+
offset = ((unsigned int)(unsigned long)buf & (PAGE_SIZE - 1));
num_pages = DIV_ROUND_UP(count + offset, PAGE_SIZE);
 
+   if (num_pages > (SIZE_MAX - sizeof(struct pagelist) -
+sizeof(struct vchiq_pagelist_info)) /
+   (sizeof(u32) + sizeof(pages[0]) +
+sizeof(struct scatterlist)))
+   return NULL;
+
pagelist_size = sizeof(struct pagelist) +
(num_pages * sizeof(u32)) +
(num_pages * sizeof(pages[0]) +
-- 
2.20.1

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

Re: [PATCH V2 1/5] Staging: rtl8192u: ieee80211: Fix coding style warning

[Dan Carpenter]

On Tue, May 14, 2019 at 09:24:38PM +0530, Puranjay Mohan wrote:
> On Tue, May 14, 2019 at 04:09:05PM +0200, Greg KH wrote:
> > On Tue, May 14, 2019 at 02:42:31PM +0530, Puranjay Mohan wrote:
> > > Remove braces around a single if statement to fix following
> > > checkpatch.pl warning.
> > > WARNING: braces {} are not necessary for single statement blocks
> > > 
> > > Signed-off-by: Puranjay Mohan 
> > > ---
> > > 
> > > V2 - remove extra blank line before the closing brace.
> > 
> > When you fix up a patch in a series, please resend the _whole_ series,
> > otherwise I have to try to piece together the different patches and put
> > them in the proper place.  When dealing with 1000+ emails a day, that's
> > a hard thing to ask a maintainer to do.
> I am sorry! 
> > So please just resend this whole thing, in a threaded email series (such
> > that they are all grouped together.  You are sending these as individual
> > emails, and so email clients do not link them, making them harder to
> > manage :(
> > 
> > thanks,
> > 
> > greg k-h
> I will resend it again properly.
> Sorry for doing mistakes everytime.

Don't stress  We all started as newbies.

regards,
dan carpenter

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

Re: [PATCH 5/5] Staging: rtl8192u: ieee80211: Use !x in place of NULL comparison

[Dan Carpenter]

On Tue, May 14, 2019 at 01:11:21AM +0530, Puranjay Mohan wrote:
> @@ -2856,7 +2856,7 @@ static int ieee80211_wpa_set_encryption(struct 
> ieee80211_device *ieee,
>   goto done;
>   }
>  
> - if (*crypt == NULL || (*crypt)->ops != ops) {
> + if (!(*crypt) || (*crypt)->ops != ops) {
 ^  ^
Please don't add these parentheses.

>   struct ieee80211_crypt_data *new_crypt;
>  
>   ieee80211_crypt_delayed_deinit(ieee, crypt);

regards,
dan carpenter

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

Re: [PATCH 3/5] Staging: rtl8192u: ieee80211: Fix coding style errors

[Dan Carpenter]

On Tue, May 14, 2019 at 01:09:50AM +0530, Puranjay Mohan wrote:
> @@ -746,12 +730,12 @@ int ieee80211_xmit(struct sk_buff *skb, struct 
> net_device *dev)
>   txb->payload_size = __cpu_to_le16(bytes);
>  
>   //if (ieee->current_network.QoS_Enable)
> - if(qos_actived)
> - {
> + if (qos_actived)
> +
>   txb->queue_index = UP2AC(skb->priority);

No blank line here either.

> - } else {
> + else
>   txb->queue_index = WME_AC_BK;
> - }
> +

Or here.

regards,
dan carpenter

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

Re: [PATCH 1/5] Staging: rtl8192u: ieee80211: Fix coding style warning

[Dan Carpenter]

On Tue, May 14, 2019 at 01:08:11AM +0530, Puranjay Mohan wrote:
> Remove braces around a single if statement to fix following
> checkpatch.pl warning.
> WARNING: braces {} are not necessary for single statement blocks
> 
> Signed-off-by: Puranjay Mohan 
> ---
>  drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c 
> b/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
> index 0e762e559675..bd97531a254f 100644
> --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
> +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
> @@ -2577,9 +2577,9 @@ static inline void ieee80211_process_probe_response(
>   spin_unlock_irqrestore(>lock, flags);
>   if 
> (is_beacon(beacon->header.frame_ctl)&_same_network(>current_network, 
> network, ieee)&&\
>   (ieee->state == IEEE80211_LINKED)) {
> - if (ieee->handle_beacon != NULL) {
> + if (ieee->handle_beacon != NULL)
>   
> ieee->handle_beacon(ieee->dev,beacon,>current_network);
> - }
> +
>   }

We don't want the blank line.  Just delete the '}'

regards,
dan carpenter

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

[PATCH] Staging: vc04_services: Fix a couple error codes

[Dan Carpenter]

These are accidentally returning positive EINVAL instead of negative
-EINVAL.  Some of the callers treat positive values as success.

Fixes: 7b3ad5abf027 ("staging: Import the BCM2835 MMAL-based V4L2 camera 
driver.")
Signed-off-by: Dan Carpenter 
---
 drivers/staging/vc04_services/bcm2835-camera/controls.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-camera/controls.c 
b/drivers/staging/vc04_services/bcm2835-camera/controls.c
index 9841c30450ce..dade79738a29 100644
--- a/drivers/staging/vc04_services/bcm2835-camera/controls.c
+++ b/drivers/staging/vc04_services/bcm2835-camera/controls.c
@@ -572,7 +572,7 @@ static int ctrl_set_image_effect(struct bm2835_mmal_dev 
*dev,
dev->colourfx.enable ? "true" : "false",
dev->colourfx.u, dev->colourfx.v,
ret, (ret == 0 ? 0 : -EINVAL));
-   return (ret == 0 ? 0 : EINVAL);
+   return (ret == 0 ? 0 : -EINVAL);
 }
 
 static int ctrl_set_colfx(struct bm2835_mmal_dev *dev,
@@ -596,7 +596,7 @@ static int ctrl_set_colfx(struct bm2835_mmal_dev *dev,
 "%s: After: mmal_ctrl:%p ctrl id:0x%x ctrl val:%d ret 
%d(%d)\n",
__func__, mmal_ctrl, ctrl->id, ctrl->val, ret,
(ret == 0 ? 0 : -EINVAL));
-   return (ret == 0 ? 0 : EINVAL);
+   return (ret == 0 ? 0 : -EINVAL);
 }
 
 static int ctrl_set_bitrate(struct bm2835_mmal_dev *dev,
-- 
2.20.1

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

Re: [PATCH v3 1/8] Staging: kpc2000: kpc_dma: Resolve trailing whitespace error reported by checkpatch

[Dan Carpenter]

On Mon, May 13, 2019 at 03:56:15PM +0530, Vandana BN wrote:
> Resolve trailing whitespace error from checkpatch.pl
> ERROR: trailing whitespace
> ---
> v2-split changes to multiple patches
> v3 - edit commit message
> ---
> 
> Signed-off-by: Vandana BN 
> ---

The Signed off by has to be before the first --- cut off line.
Everything after the cut off is removed from the commit message.

>  drivers/staging/kpc2000/kpc_dma/dma.c |  86 ++---
>  drivers/staging/kpc2000/kpc_dma/fileops.c | 114 +-
>  .../staging/kpc2000/kpc_dma/kpc_dma_driver.c  |  46 +++
>  .../staging/kpc2000/kpc_dma/kpc_dma_driver.h  |  16 +--
>  4 files changed, 131 insertions(+), 131 deletions(-)
> 


regards,
dan carpenter

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

Re: [PATCH] drivers: staging :rtl8723bs :os_dep Remove Unneeded variable ret

[Dan Carpenter]

Please "drivers" out of the subject line.  We know it's drivers, so that
doesn't add any information.  The "staging: " bit tells you which git
tree this path is in, and the "rtl8723bs: " tells you which driver it
is.

regards,
dan carpenter

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