[staging:staging-testing 87/153] drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c:686 vchiq_initialise() error: we previously assumed 'state' could be null (see line 681)
tree: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
staging-testing
head: 5e8cdb6f6ebe28976876ab04995a5d3779b85082
commit: e70f17ed997cb7ee6c34089f2cdc2a9edc886503 [87/153] staging:
vc04_services: Drop vchiq_log_error() in favour of dev_err
config: csky-randconfig-r081-20231218
(https://download.01.org/0day-ci/archive/20231219/202312190038.zuex32pb-...@intel.com/config)
compiler: csky-linux-gcc (GCC) 13.2.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Reported-by: Dan Carpenter
| Closes: https://lore.kernel.org/r/202312190038.zuex32pb-...@intel.com/
smatch warnings:
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c:686
vchiq_initialise() error: we previously assumed 'state' could be null (see line
681)
vim +/state +686 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
abf2836a381a307 Stefan Wahren2021-04-25 668 int
vchiq_initialise(struct vchiq_instance **instance_out)
5c5e6ef6287cbf3 Arnd Bergmann2018-02-02 669 {
2d0a0291135fd2f Dominic Braun2018-12-14 670struct vchiq_state
*state;
4ddf9a2555caf21 Jamal Shareef2019-11-05 671struct vchiq_instance
*instance = NULL;
abf2836a381a307 Stefan Wahren2021-04-25 672int i, ret;
5c5e6ef6287cbf3 Arnd Bergmann2018-02-02 673
3da8757576ef789 Amarjargal Gundjalam 2020-10-28 674/*
3da8757576ef789 Amarjargal Gundjalam 2020-10-28 675 * VideoCore may not be
ready due to boot up timing.
5c5e6ef6287cbf3 Arnd Bergmann2018-02-02 676 * It may never be
ready if kernel and firmware are mismatched,so don't
5c5e6ef6287cbf3 Arnd Bergmann2018-02-02 677 * block forever.
5c5e6ef6287cbf3 Arnd Bergmann2018-02-02 678 */
5c5e6ef6287cbf3 Arnd Bergmann2018-02-02 679for (i = 0; i <
VCHIQ_INIT_RETRIES; i++) {
5c5e6ef6287cbf3 Arnd Bergmann2018-02-02 680state =
vchiq_get_state();
5c5e6ef6287cbf3 Arnd Bergmann2018-02-02 @681if (state)
We exit early if state is valid
5c5e6ef6287cbf3 Arnd Bergmann2018-02-02 682break;
81244ba0f03691a Stefan Wahren2018-03-31 683
usleep_range(500, 600);
5c5e6ef6287cbf3 Arnd Bergmann2018-02-02 684}
5c5e6ef6287cbf3 Arnd Bergmann2018-02-02 685if (i ==
VCHIQ_INIT_RETRIES) {
e70f17ed997cb7e Umang Jain 2023-12-05 @686
dev_err(state->dev, "core: %s: Videocore not initialized\n", __func__);
^^
state is NULL at this point.
abf2836a381a307 Stefan Wahren2021-04-25 687ret = -ENOTCONN;
5c5e6ef6287cbf3 Arnd Bergmann2018-02-02 688goto failed;
5c5e6ef6287cbf3 Arnd Bergmann2018-02-02 689} else if (i > 0) {
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[staging:staging-testing 17/17] drivers/staging/vme_user/vme.c:296 vme_slave_request() error: we previously assumed 'slave_image' could be null (see line 297)
Hi Jinjie,
FYI, the error/warning was bisected to this commit, please ignore it if it's
irrelevant.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
staging-testing
head: 4c5ba1d7a93e098aececbf93afbdd7add98ec6f3
commit: 4c5ba1d7a93e098aececbf93afbdd7add98ec6f3 [17/17] staging: vme_user: Use
list_for_each_entry() helper
config: x86_64-randconfig-161-20230913
(https://download.01.org/0day-ci/archive/20230914/202309140330.fgoxorhe-...@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce:
(https://download.01.org/0day-ci/archive/20230914/202309140330.fgoxorhe-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Reported-by: Dan Carpenter
| Closes: https://lore.kernel.org/r/202309140330.fgoxorhe-...@intel.com/
smatch warnings:
drivers/staging/vme_user/vme.c:296 vme_slave_request() error: we previously
assumed 'slave_image' could be null (see line 297)
drivers/staging/vme_user/vme.c:492 vme_master_request() error: we previously
assumed 'master_image' could be null (see line 493)
drivers/staging/vme_user/vme.c:866 vme_dma_request() error: we previously
assumed 'dma_ctrlr' could be null (see line 867)
drivers/staging/vme_user/vme.c:1460 vme_lm_request() error: we previously
assumed 'lm' could be null (see line 1461)
vim +/slave_image +296 drivers/staging/vme_user/vme.c
6af04b065b048e drivers/staging/vme/vme.c Martyn Welch2011-12-01 281
struct vme_resource *vme_slave_request(struct vme_dev *vdev, u32 address,
6af04b065b048e drivers/staging/vme/vme.c Martyn Welch2011-12-01 282
u32 cycle)
a17a75e2666f71 drivers/staging/vme/vme.c Martyn Welch2009-07-31 283 {
a17a75e2666f71 drivers/staging/vme/vme.c Martyn Welch2009-07-31 284
struct vme_bridge *bridge;
a17a75e2666f71 drivers/staging/vme/vme.c Martyn Welch2009-07-31 285
struct vme_slave_resource *allocated_image = NULL;
a17a75e2666f71 drivers/staging/vme/vme.c Martyn Welch2009-07-31 286
struct vme_slave_resource *slave_image = NULL;
a17a75e2666f71 drivers/staging/vme/vme.c Martyn Welch2009-07-31 287
struct vme_resource *resource = NULL;
a17a75e2666f71 drivers/staging/vme/vme.c Martyn Welch2009-07-31 288
8f966dc444b11a drivers/staging/vme/vme.c Manohar Vanga 2011-09-26 289
bridge = vdev->bridge;
61282c04984e40 drivers/vme/vme.c Markus Elfring 2017-08-24 290
if (!bridge) {
a17a75e2666f71 drivers/staging/vme/vme.c Martyn Welch2009-07-31 291
printk(KERN_ERR "Can't find VME bus\n");
a17a75e2666f71 drivers/staging/vme/vme.c Martyn Welch2009-07-31 292
goto err_bus;
a17a75e2666f71 drivers/staging/vme/vme.c Martyn Welch2009-07-31 293
}
a17a75e2666f71 drivers/staging/vme/vme.c Martyn Welch2009-07-31 294
a17a75e2666f71 drivers/staging/vme/vme.c Martyn Welch2009-07-31 295
/* Loop through slave resources */
4c5ba1d7a93e09 drivers/staging/vme_user/vme.c Jinjie Ruan 2023-08-23 @296
list_for_each_entry(slave_image, >slave_resources, list) {
^^^
This is the iterator.
61282c04984e40 drivers/vme/vme.c Markus Elfring 2017-08-24 @297
if (!slave_image) {
And list iterators are never NULL. Please remove this if statement.
ead1f3e301e2d8 drivers/staging/vme/vme.c Martyn Welch2009-12-15 298
printk(KERN_ERR "Registered NULL Slave resource\n");
a17a75e2666f71 drivers/staging/vme/vme.c Martyn Welch2009-07-31 299
continue;
a17a75e2666f71 drivers/staging/vme/vme.c Martyn Welch2009-07-31 300
}
a17a75e2666f71 drivers/staging/vme/vme.c Martyn Welch2009-07-31 301
a17a75e2666f71 drivers/staging/vme/vme.c Martyn Welch2009-07-31 302
/* Find an unlocked and compatible image */
886953e9b70bcb drivers/staging/vme/vme.c Emilio G. Cota 2010-11-12 303
mutex_lock(_image->mtx);
a17a75e2666f71 drivers/staging/vme/vme.c Martyn Welch2009-07-31 304
if (((slave_image->address_attr & address) == address) &&
a17a75e2666f71 drivers/staging/vme/vme.c Martyn Welch2009-07-31 305
((slave_image->cycle_attr & cycle) == cycle) &&
a17a75e2666f71 drivers/staging/vme/vme.c Martyn Welch2009-07-31 306
(slave_image->locked == 0)) {
a17a75e2666f71 drivers/staging/vme/vme.c Martyn Welch2009-07-31 307
slave_image->locked = 1;
886953e9b70bcb drivers/staging/vme/vme.c Emilio G. Cota 2010-11-12 308
mutex_unlock(_image->mtx);
a17a75e26
Re: [PATCH] staging: gs_fpgaboot: revert removed board specific code
On Thu, Feb 03, 2022 at 09:40:27PM -0800, Tong Zhang wrote:
> gs_fpgaboot is currently useless since the board specific code is
> removed in 06a3fab941da. Loading the driver will always fail since
> xl_init_io() always returns -1. This driver is broken since 2014 and I
> doubt anyone is actually using it, we could either remove it or revert
> to the previous working version.
>
> $ modprobe gs_fpga
> GPIO INIT FAIL!!
>
> This patch reverts previously removed code and adds a Kconfig to make
> this board selectable for PPC_85xx processors.
>
> Fixes: 06a3fab941da ("staging: gs_fpgaboot: remove checks for
> CONFIG_B4860G100")
> Signed-off-by: Tong Zhang
The Fixes tag is not really accurate. The code has never worked. It
should be:
Fixes: e7185c6958ee ("staging: fpgaboot: Xilinx FPGA firmware download driver")
I assume you don't really have this hardware and you're just modprobing
drivers to as part of testing? If you have this hardware then we can
preserve it. Otherwise we should just delete the whole driver. It's
been 8 years and no one has noticed that it doesn't probe.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 0/4] binder: Prevent untranslated sender data from being copied to target
On Tue, Nov 30, 2021 at 10:51:48AM -0800, Todd Kjos wrote: > v2: > - add "binder: fix handling of error during copy" to fix > bug noticed by Dan Carpenter > - address Dan Carpenter's comments Thanks! regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 1/1] staging: ion: Prevent incorrect reference counting behavour
On Fri, Nov 26, 2021 at 10:33:35AM +, Lee Jones wrote:
> Supply additional checks in order to prevent unexpected results.
>
> Fixes: b892bf75b2034 ("ion: Switch ion to use dma-buf")
> Suggested-by: Dan Carpenter
> Signed-off-by: Lee Jones
> ---
> Destined for v4.4.y and v4.9.y
Thanks!
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] staging: ion: Prevent incorrect reference counting behavour
On Fri, Nov 26, 2021 at 08:56:27AM +, Lee Jones wrote:
> On Fri, 26 Nov 2021, Dan Carpenter wrote:
>
> > On Thu, Nov 25, 2021 at 06:18:22PM +0300, Dan Carpenter wrote:
> > > I had thought that ->kmap_cnt was a regular int and not an unsigned
> > > int, but I would have to pull a stable tree to see where I misread the
> > > code.
> >
> > I was looking at (struct ion_buffer)->kmap_cnt but this is
> > (struct ion_handle)->kmap_cnt. I'm not sure how those are related but
> > it makes me nervous that one can go higher than the other. Also both
> > probably need overflow protection.
> >
> > So I guess I would just do something like:
> >
> > diff --git a/drivers/staging/android/ion/ion.c
> > b/drivers/staging/android/ion/ion.c
> > index 806e9b30b9dc8..e8846279b33b5 100644
> > --- a/drivers/staging/android/ion/ion.c
> > +++ b/drivers/staging/android/ion/ion.c
> > @@ -489,6 +489,8 @@ static void *ion_buffer_kmap_get(struct ion_buffer
> > *buffer)
> > void *vaddr;
> >
> > if (buffer->kmap_cnt) {
> > + if (buffer->kmap_cnt == INT_MAX)
> > + return ERR_PTR(-EOVERFLOW);
> > buffer->kmap_cnt++;
> > return buffer->vaddr;
> > }
> > @@ -509,6 +511,8 @@ static void *ion_handle_kmap_get(struct ion_handle
> > *handle)
> > void *vaddr;
> >
> > if (handle->kmap_cnt) {
> > + if (handle->kmap_cnt == INT_MAX)
> > + return ERR_PTR(-EOVERFLOW);
> > handle->kmap_cnt++;
> > return buffer->vaddr;
> > }
>
> Which is all well and good until somebody changes the type.
It would only break if they change it to something smaller than an int.
I have zero sympathy for them if they do that.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] staging: ion: Prevent incorrect reference counting behavour
On Thu, Nov 25, 2021 at 06:18:22PM +0300, Dan Carpenter wrote:
> I had thought that ->kmap_cnt was a regular int and not an unsigned
> int, but I would have to pull a stable tree to see where I misread the
> code.
I was looking at (struct ion_buffer)->kmap_cnt but this is
(struct ion_handle)->kmap_cnt. I'm not sure how those are related but
it makes me nervous that one can go higher than the other. Also both
probably need overflow protection.
So I guess I would just do something like:
diff --git a/drivers/staging/android/ion/ion.c
b/drivers/staging/android/ion/ion.c
index 806e9b30b9dc8..e8846279b33b5 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -489,6 +489,8 @@ static void *ion_buffer_kmap_get(struct ion_buffer *buffer)
void *vaddr;
if (buffer->kmap_cnt) {
+ if (buffer->kmap_cnt == INT_MAX)
+ return ERR_PTR(-EOVERFLOW);
buffer->kmap_cnt++;
return buffer->vaddr;
}
@@ -509,6 +511,8 @@ static void *ion_handle_kmap_get(struct ion_handle *handle)
void *vaddr;
if (handle->kmap_cnt) {
+ if (handle->kmap_cnt == INT_MAX)
+ return ERR_PTR(-EOVERFLOW);
handle->kmap_cnt++;
return buffer->vaddr;
}
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] staging: ion: Prevent incorrect reference counting behavour
On Thu, Nov 25, 2021 at 03:07:37PM +, Lee Jones wrote:
> On Thu, 25 Nov 2021, Dan Carpenter wrote:
>
> > On Thu, Nov 25, 2021 at 02:20:04PM +, Lee Jones wrote:
> > > Supply additional checks in order to prevent unexpected results.
> > >
> > > Fixes: b892bf75b2034 ("ion: Switch ion to use dma-buf")
> > > Signed-off-by: Lee Jones
> > > ---
> > > Should be back-ported from v4.9 and earlier.
> > >
> > > drivers/staging/android/ion/ion.c | 5 +
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/staging/android/ion/ion.c
> > > b/drivers/staging/android/ion/ion.c
> > > index 806e9b30b9dc8..402b74f5d7e69 100644
> > > --- a/drivers/staging/android/ion/ion.c
> > > +++ b/drivers/staging/android/ion/ion.c
> > > @@ -29,6 +29,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -509,6 +510,10 @@ static void *ion_handle_kmap_get(struct ion_handle
> > > *handle)
> > > void *vaddr;
> > >
> > > if (handle->kmap_cnt) {
> > > + if (check_add_overflow(handle->kmap_cnt,
> > > +(unsigned int) 1, >kmap_cnt))
> > ^
> >
> > > + return ERR_PTR(-EOVERFLOW);
> > > +
> > > handle->kmap_cnt++;
> > ^^^
> > This will not do what you want at all. It's a double increment on the
> > success path and it leave handle->kmap_cnt overflowed on failure path.
>
> I read the helper to take copies of the original variables.
>
> #define __unsigned_add_overflow(a, b, d) ({ \
> typeof(a) __a = (a);\
> typeof(b) __b = (b);\
> typeof(d) __d = (d);\
> (void) (&__a == &__b); \
> (void) (&__a == __d); \
> *__d = __a + __b; \
This assignment sets handle->kmap_cnt to the overflowed value.
> *__d < __a; \
> })
>
> Maybe I misread it.
>
> So the original patch [0] was better?
>
> [0]
> https://lore.kernel.org/stable/20211125120234.67987-1-lee.jo...@linaro.org/
The original at least worked. :P
You're catching me right as I'm knocking off for the day so I'm not
sure how to write this code. I had thought that ->kmap_cnt was a
regular int and not an unsigned int, but I would have to pull a stable
tree to see where I misread the code.
I'll look at this tomorrow Nairobi time, but I expect by then you'll
already have it all figured out.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] staging: ion: Prevent incorrect reference counting behavour
On Thu, Nov 25, 2021 at 02:20:04PM +, Lee Jones wrote:
> Supply additional checks in order to prevent unexpected results.
>
> Fixes: b892bf75b2034 ("ion: Switch ion to use dma-buf")
> Signed-off-by: Lee Jones
> ---
> Should be back-ported from v4.9 and earlier.
>
> drivers/staging/android/ion/ion.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/staging/android/ion/ion.c
> b/drivers/staging/android/ion/ion.c
> index 806e9b30b9dc8..402b74f5d7e69 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -29,6 +29,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -509,6 +510,10 @@ static void *ion_handle_kmap_get(struct ion_handle
> *handle)
> void *vaddr;
>
> if (handle->kmap_cnt) {
> + if (check_add_overflow(handle->kmap_cnt,
> +(unsigned int) 1, >kmap_cnt))
^
> + return ERR_PTR(-EOVERFLOW);
> +
> handle->kmap_cnt++;
^^^
This will not do what you want at all. It's a double increment on the
success path and it leave handle->kmap_cnt overflowed on failure path.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/3] binder: read pre-translated fds from sender buffer
On Wed, Nov 24, 2021 at 12:33:20PM -0800, Todd Kjos wrote: > I agree -- if copy_from_user() for some reason doesn't copy the whole > buffer, it might return a positive integer. Then it would skip > binder_translate_fd(), but not return. That should probably be > something like: > > if (ret) > return ret > 0 ? -EINVAL : ret; > > Will fix in next version. It should really be a separate patch at the start of the series because it's from the original code and unrelated. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/3] binder: avoid potential data leakage when copying txn
On Tue, Nov 23, 2021 at 11:17:35AM -0800, Todd Kjos wrote:
> Transactions are copied from the sender to the target
> first and objects like BINDER_TYPE_PTR and BINDER_TYPE_FDA
> are then fixed up. This means there is a short period where
> the sender's version of these objects are visible to the
> target prior to the fixups.
>
> Instead of copying all of the data first, copy data only
> after any needed fixups have been applied.
>
This patch needs a fixes tag.
So this patch basically fixes the simple part of the info leak and
patch 3 fixes the complicated part. Have I understood that correctly?
> @@ -2956,7 +2984,11 @@ static void binder_transaction(struct binder_proc
> *proc,
> }
> ret = binder_translate_fd_array(fda, parent, t, thread,
> in_reply_to);
> - if (ret < 0) {
> + if (ret < 0 ||
> + binder_alloc_copy_to_buffer(_proc->alloc,
> + t->buffer,
> + object_offset,
> + fda, sizeof(*fda))) {
> return_error = BR_FAILED_REPLY;
> return_error_param = ret;
"ret" is not a negative error code if binder_translate_fd_array()
succeeds but binder_alloc_copy_to_buffer() fails.
> return_error_line = __LINE__;
> @@ -3028,6 +3060,19 @@ static void binder_transaction(struct binder_proc
> *proc,
> goto err_bad_object_type;
> }
> }
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/3] binder: defer copies of pre-patched txn data
On Tue, Nov 23, 2021 at 11:17:37AM -0800, Todd Kjos wrote:
> +static int binder_do_deferred_txn_copies(struct binder_alloc *alloc,
> + struct binder_buffer *buffer,
> + struct list_head *sgc_head,
> + struct list_head *pf_head)
> +{
> + int ret = 0;
> + struct list_head *entry, *tmp;
> + struct binder_ptr_fixup *pf =
> + list_first_entry_or_null(pf_head, struct binder_ptr_fixup,
> + node);
> +
> + list_for_each_safe(entry, tmp, sgc_head) {
^^^
All the list_for_each() loops can be changed to list_for_each_entry().
list_for_each_entry_safe(sgc, tmp, sgc_head, node) {
regards,
dan carpenter
> + size_t bytes_copied = 0;
> + struct binder_sg_copy *sgc =
> + container_of(entry, struct binder_sg_copy, node);
> +
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/3] binder: read pre-translated fds from sender buffer
_start_offset) /
> @@ -2982,8 +2988,28 @@ static void binder_transaction(struct binder_proc
> *proc,
> return_error_line = __LINE__;
> goto err_bad_parent;
> }
> - ret = binder_translate_fd_array(fda, parent, t, thread,
> - in_reply_to);
> +
> + /*
> + * We need to read the user version of the parent
> + * object to get the original user offset
> + */
> + user_parent_size =
> + binder_get_object(proc, user_buffer, t->buffer,
> + parent_offset, _object);
> + if (user_parent_size != sizeof(user_object.bbo)) {
> + binder_user_error("%d:%d invalid ptr object
> size: %lld vs %lld\n",
Apparently %lld breaks the build on my .config. The correct format for
size_t is %zd.
> + proc->pid, thread->pid,
> + user_parent_size,
> + sizeof(user_object.bbo));
> + return_error = BR_FAILED_REPLY;
> + return_error_param = -EINVAL;
> + return_error_line = __LINE__;
> + goto err_bad_parent;
> + }
> + ret = binder_translate_fd_array(fda, user_buffer,
> + parent,
> + _object.bbo, t,
> + thread, in_reply_to);
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/3] binder: defer copies of pre-patched txn data
@pf_head if there is an
> + * error.
> + */
> +static void binder_cleanup_deferred_txn_lists(struct list_head *sgc_head,
> + struct list_head *pf_head)
> +{
> + struct list_head *entry, *tmp;
> +
> + list_for_each_safe(entry, tmp, sgc_head) {
> + struct binder_sg_copy *sgc =
> + container_of(entry, struct binder_sg_copy, node);
> + list_del(>node);
> + kfree(sgc);
> + }
> + list_for_each_safe(entry, tmp, pf_head) {
> + struct binder_ptr_fixup *pf =
> + container_of(entry, struct binder_ptr_fixup, node);
> + list_del(>node);
> + kfree(pf);
> + }
> +}
> +
> +/**
> + * binder_defer_copy() - queue a scatter-gather buffer for copy
> + * @sgc_head:list_head of scatter-gather copy list
> + * @offset: binder buffer offset in target process
> + * @uaddr: user address in source process
> + * @length: bytes to copy
> + *
> + * Specify a scatter-gather block to be copied. The actual copy must
> + * be deferred until all the needed fixups are identified and queued.
> + * Then the copy and fixups are done together so un-translated values
> + * from the source are never visible in the target buffer.
> + *
> + * We are guaranteed that repeated calls to this function will have
> + * monotonically increasing @offset values so the list will naturally
> + * be ordered.
> + *
> + * Return: 0=success, else -errno
> + */
> +static int binder_defer_copy(struct list_head *sgc_head, binder_size_t
> offset,
> + const void __user *uaddr, size_t length)
> +{
> + struct binder_sg_copy *bc = kzalloc(sizeof(*bc), GFP_KERNEL);
> +
> + if (!bc)
> + return -ENOMEM;
> +
> + bc->offset = offset;
> + bc->uaddr = uaddr;
> + bc->length = length;
> + INIT_LIST_HEAD(>node);
> +
> + /*
> + * We are guaranteed that the deferred copies are in-order
> + * so just add to the tail.
> + */
> + list_add_tail(>node, sgc_head);
> +
> + return 0;
> +}
> +
> +/**
> + * binder_add_fixup() - queue a fixup to be applied to sg copy
> + * @pf_head: list_head of binder ptr fixup list
> + * @offset: binder buffer offset in target process
> + * @fixup: bytes to be copied for fixup
> + * @skip_size: bytes to skip when copying (fixup will be applied later)
> + *
> + * Add the specified fixup to a list ordered by @offset. When copying
> + * the scatter-gather buffers, the fixup will be copied instead of
> + * data from the source buffer. For BINDER_TYPE_FDA fixups, the fixup
> + * will be applied later (in target process context), so we just skip
> + * the bytes specified by @skip_size. If @skip_size is 0, we copy the
> + * value in @fixup.
> + *
> + * This function is called *mostly* in @offset order, but there are
> + * exceptions. Since out-of-order inserts are relatively uncommon,
> + * we insert the new element by searching backward from the tail of
> + * the list.
> + *
> + * Return: 0=success, else -errno
> + */
> +static int binder_add_fixup(struct list_head *pf_head, binder_size_t offset,
> + binder_uintptr_t fixup, size_t skip_size)
> +{
> + struct binder_ptr_fixup *pf = kzalloc(sizeof(*pf), GFP_KERNEL);
> + struct list_head *tmp;
> +
> + if (!pf)
> + return -ENOMEM;
> +
> + pf->offset = offset;
> + pf->fixup_data = fixup;
> + pf->skip_size = skip_size;
> + INIT_LIST_HEAD(>node);
> +
> + /* Fixups are *mostly* added in-order, but there are some
> + * exceptions. Look backwards through list for insertion point.
> + */
> + if (!list_empty(pf_head)) {
This condition is not required. If list is empty we add it to the tail,
but when there is only one item on the list, the first and last item are
going to be the same.
> + list_for_each_prev(tmp, pf_head) {
> + struct binder_ptr_fixup *tmppf =
> + list_entry(tmp, struct binder_ptr_fixup, node);
> +
> + if (tmppf->offset < pf->offset) {
> + list_add(>node, tmp);
> + return 0;
> + }
> + }
> + /*
> + * if we get here, then the new offset is the lowest so
> + * insert at the head
> + */
> + list_add(>node, pf_head);
> + return 0;
> + }
> + list_add_tail(>node, pf_head);
> + return 0;
> +}
> +
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] staging: vt6655: refactor camelcase uCurrRSSI to current_rssi
On Thu, Nov 18, 2021 at 09:27:18PM +0100, Alberto Merciai wrote: > Replace camelcase variable "uCurrRSSI" (current Received Signal Strength > Indicator) into linux kernel coding style equivalent > variable "current_rssi". > > Signed-off-by: Alberto Merciai > --- > > v2 > - correct mailing list Are you using the staging-next tree? regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v11 4/4] drm/bridge: anx7625: add HDMI audio function
This is a super awkward way to resend a patch series. Next time, just start a new thread and put [PATCH RESEND] in the subject. I am sorry that no one responded to your thread. :/ regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binder: don't detect sender/target during buffer cleanup
On Fri, Oct 15, 2021 at 04:38:11PM -0700, Todd Kjos wrote: > When freeing txn buffers, binder_transaction_buffer_release() > attempts to detect whether the current context is the target by > comparing current->group_leader to proc->tsk. This is an unreliable > test. Instead explicitly pass an 'is_failure' boolean. > > Detecting the sender was being used as a way to tell if the > transaction failed to be sent. When cleaning up after > failing to send a transaction, there is no need to close > the fds associated with a BINDER_TYPE_FDA object. Now > 'is_failure' can be used to accurately detect this case. > It's really hard for me to understand what this bug looks like to the user? Is it a memory leak or do we free the wrong thing? regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 2/3] binder: use cred instead of task for getsecid
On Mon, Oct 11, 2021 at 02:59:13PM -0700, Casey Schaufler wrote:
> On 10/11/2021 2:33 PM, Paul Moore wrote:
> > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos wrote:
> >> Use the 'struct cred' saved at binder_open() to lookup
> >> the security ID via security_cred_getsecid(). This
> >> ensures that the security context that opened binder
> >> is the one used to generate the secctx.
> >>
> >> Fixes: ec74136ded79 ("binder: create node flag to request sender's
> >> security context")
> >> Signed-off-by: Todd Kjos
> >> Suggested-by: Stephen Smalley
> >> Reported-by: kernel test robot
> >> Cc: sta...@vger.kernel.org # 5.4+
> >> ---
> >> v3: added this patch to series
> >> v4: fix build-break for !CONFIG_SECURITY
> >>
> >> drivers/android/binder.c | 11 +--
> >> include/linux/security.h | 4
> >> 2 files changed, 5 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> >> index ca599ebdea4a..989afd0804ca 100644
> >> --- a/drivers/android/binder.c
> >> +++ b/drivers/android/binder.c
> >> @@ -2722,16 +2722,7 @@ static void binder_transaction(struct binder_proc
> >> *proc,
> >> u32 secid;
> >> size_t added_size;
> >>
> >> - /*
> >> -* Arguably this should be the task's subjective LSM secid
> >> but
> >> -* we can't reliably access the subjective creds of a task
> >> -* other than our own so we must use the objective creds,
> >> which
> >> -* are safe to access. The downside is that if a task is
> >> -* temporarily overriding it's creds it will not be
> >> reflected
> >> -* here; however, it isn't clear that binder would handle
> >> that
> >> -* case well anyway.
> >> -*/
> >> - security_task_getsecid_obj(proc->tsk, );
> >> + security_cred_getsecid(proc->cred, );
> >> ret = security_secid_to_secctx(secid, , _sz);
> >> if (ret) {
> >> return_error = BR_FAILED_REPLY;
> >> diff --git a/include/linux/security.h b/include/linux/security.h
> >> index 6344d3362df7..f02cc0211b10 100644
> >> --- a/include/linux/security.h
> >> +++ b/include/linux/security.h
> >> @@ -1041,6 +1041,10 @@ static inline void security_transfer_creds(struct
> >> cred *new,
> >> {
> >> }
> >>
> >> +static inline void security_cred_getsecid(const struct cred *c, u32
> >> *secid)
> >> +{
> >> +}
> > Since security_cred_getsecid() doesn't return an error code we should
> > probably set the secid to 0 in this case, for example:
> >
> > static inline void security_cred_getsecid(...)
> > {
> > *secid = 0;
> > }
>
> If CONFIG_SECURITY is unset there shouldn't be any case where
> the secid value is ever used for anything. Are you suggesting that
> it be set out of an abundance of caution?
The security_secid_to_secctx() function is probably inlined so probably
KMSan will not warn about this. But Smatch will warn about passing
unitialized variables. You probably wouldn't recieve and email about
it, and I would just add an exception that security_cred_getsecid()
should be ignored.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/32] staging/wfx: usual maintenance
On Mon, Sep 13, 2021 at 03:01:31PM +0200, Jerome Pouiller wrote: > From: Jérôme Pouiller > > Hi, > > The following PR contains now usual maintenance for the wfx driver. I have > more-or-less sorted the patches by importance: > - the first ones and the two last ones are fixes for a few corner-cases > reported by users > - the patches 9 and 10 add support for CSA and TDLS > - then the end of the series is mostly cosmetics and nitpicking > > I have wait longer than I initially wanted before to send this PR. It is > because didn't want to conflict with the PR currently in review[1] to > relocate this driver into the main tree. However, this PR started to be > very large and nothing seems to move on main-tree side so I decided to not > wait longer. > > Kalle, I am going to send a new version of [1] as soon as this PR will be > accepted. I hope you will have time to review it one day :-). > > [1] > https://lore.kernel.org/all/20210315132501.441681-1-jerome.pouil...@silabs.com/ > > v3: > - Fix patch 11 and drop patch 33 (Dan) > - Fix one missing C99 comment > - Drop useless WARN_ON() (Dan) Thanks! Only dropping patch 11 and 33 was really required, but I'm happy you dropped the WARN_ON() as well. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 03/33] staging: wfx: ignore PS when STA/AP share same channel
On Mon, Sep 13, 2021 at 12:36:25PM +0200, Jérôme Pouiller wrote:
> On Monday 13 September 2021 11:33:28 CEST Dan Carpenter wrote:
> > On Mon, Sep 13, 2021 at 10:30:15AM +0200, Jerome Pouiller wrote:
> > > diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
> > > index 5de9ccf02285..aff0559653bf 100644
> > > --- a/drivers/staging/wfx/sta.c
> > > +++ b/drivers/staging/wfx/sta.c
> > > @@ -154,18 +154,26 @@ static int wfx_get_ps_timeout(struct wfx_vif *wvif,
> > > bool *enable_ps)
> > > chan0 = wdev_to_wvif(wvif->wdev,
> > > 0)->vif->bss_conf.chandef.chan;
> > > if (wdev_to_wvif(wvif->wdev, 1))
> > > chan1 = wdev_to_wvif(wvif->wdev,
> > > 1)->vif->bss_conf.chandef.chan;
> > > - if (chan0 && chan1 && chan0->hw_value != chan1->hw_value &&
> > > - wvif->vif->type != NL80211_IFTYPE_AP) {
> > > - // It is necessary to enable powersave if channels
> > > - // are different.
> > > - if (enable_ps)
> > > - *enable_ps = true;
> > > - if (wvif->wdev->force_ps_timeout > -1)
> > > - return wvif->wdev->force_ps_timeout;
> > > - else if (wfx_api_older_than(wvif->wdev, 3, 2))
> > > - return 0;
> > > - else
> > > - return 30;
> > > + if (chan0 && chan1 && wvif->vif->type != NL80211_IFTYPE_AP) {
> > > + if (chan0->hw_value == chan1->hw_value) {
> > > + // It is useless to enable PS if channels are the
> > > same.
> > > + if (enable_ps)
> > > + *enable_ps = false;
> > > + if (wvif->vif->bss_conf.assoc &&
> > > wvif->vif->bss_conf.ps)
> > > + dev_info(wvif->wdev->dev, "ignoring
> > > requested PS mode");
> > > + return -1;
> >
> > I can't be happy about this -1 return or how it's handled in the caller.
> > There is already a -1 return so it's not really a new bug, though...
>
> I see what you mean. However, I remember it is easy to break things
> here and I don't want to change that in a rush. So, I would prefer to
> solve that in a further PR.
Yes. That's fine. The return -1 was already there.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 13/33] staging: wfx: update with the firmware API 3.8
On Mon, Sep 13, 2021 at 10:30:25AM +0200, Jerome Pouiller wrote: > From: Jérôme Pouiller > > The firmware API 3.8 introduces new statistic counters. These changes > are backward compatible. > > Signed-off-by: Jérôme Pouiller > --- > drivers/staging/wfx/debug.c | 3 +++ > drivers/staging/wfx/hif_api_mib.h | 5 - > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/wfx/debug.c b/drivers/staging/wfx/debug.c > index eedada78c25f..e67ca0d818ba 100644 > --- a/drivers/staging/wfx/debug.c > +++ b/drivers/staging/wfx/debug.c > @@ -109,6 +109,9 @@ static int wfx_counters_show(struct seq_file *seq, void > *v) > > PUT_COUNTER(rx_beacon); > PUT_COUNTER(miss_beacon); > + PUT_COUNTER(rx_dtim); > + PUT_COUNTER(rx_dtim_aid0_clr); > + PUT_COUNTER(rx_dtim_aid0_set); > > #undef PUT_COUNTER Not related to the patch but the PUT_COUNTER macro should be called something like PRINT_COUNTER. It's not a get/put API. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 12/33] staging: wfx: simplify API coherency check
On Mon, Sep 13, 2021 at 10:30:24AM +0200, Jerome Pouiller wrote: > From: Jérôme Pouiller > > The 'channel' argument of hif_join() should never be NULL. hif_join() > does not have the responsibility to recover bug of caller. A call to > WARN() at the beginning of the function reminds this constraint to the > developer. > > In current code, if the argument channel is NULL, memory leaks. The new > code just emit a warning and does not give the illusion that it is > supported (and indeed a Oops will probably raise a few lines below). > > Signed-off-by: Jérôme Pouiller > --- > drivers/staging/wfx/hif_tx.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c > index 14b7e047916e..6ffbae32028b 100644 > --- a/drivers/staging/wfx/hif_tx.c > +++ b/drivers/staging/wfx/hif_tx.c > @@ -299,10 +299,9 @@ int hif_join(struct wfx_vif *wvif, const struct > ieee80211_bss_conf *conf, > > WARN_ON(!conf->beacon_int); > WARN_ON(!conf->basic_rates); > + WARN_ON(!channel); This fine. I'm not trying to make people redo their patches especially when you're doing a great job as a maintainer. But generally these WARN_ON()s are pointless. It's never going to happen and if we try to handle all the thing which will not happen that's an impossible task. But specificically with NULL dereferences, the WARN() will generate a stack trace and also the Oops will generate a stack trace. It's duplicative. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 11/33] staging: wfx: relax the PDS existence constraint
On Mon, Sep 13, 2021 at 10:30:23AM +0200, Jerome Pouiller wrote: > @@ -395,9 +395,7 @@ int wfx_probe(struct wfx_dev *wdev) > > dev_dbg(wdev->dev, "sending configuration file %s\n", > wdev->pdata.file_pds); > - err = wfx_send_pdata_pds(wdev); > - if (err < 0) > - goto err0; > + wfx_send_pdata_pds(wdev); You revert this change in patch 33 so let's drop this and 33 both. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 03/33] staging: wfx: ignore PS when STA/AP share same channel
On Mon, Sep 13, 2021 at 10:30:15AM +0200, Jerome Pouiller wrote:
> diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
> index 5de9ccf02285..aff0559653bf 100644
> --- a/drivers/staging/wfx/sta.c
> +++ b/drivers/staging/wfx/sta.c
> @@ -154,18 +154,26 @@ static int wfx_get_ps_timeout(struct wfx_vif *wvif,
> bool *enable_ps)
> chan0 = wdev_to_wvif(wvif->wdev, 0)->vif->bss_conf.chandef.chan;
> if (wdev_to_wvif(wvif->wdev, 1))
> chan1 = wdev_to_wvif(wvif->wdev, 1)->vif->bss_conf.chandef.chan;
> - if (chan0 && chan1 && chan0->hw_value != chan1->hw_value &&
> - wvif->vif->type != NL80211_IFTYPE_AP) {
> - // It is necessary to enable powersave if channels
> - // are different.
> - if (enable_ps)
> - *enable_ps = true;
> - if (wvif->wdev->force_ps_timeout > -1)
> - return wvif->wdev->force_ps_timeout;
> - else if (wfx_api_older_than(wvif->wdev, 3, 2))
> - return 0;
> - else
> - return 30;
> + if (chan0 && chan1 && wvif->vif->type != NL80211_IFTYPE_AP) {
> + if (chan0->hw_value == chan1->hw_value) {
> + // It is useless to enable PS if channels are the same.
> + if (enable_ps)
> + *enable_ps = false;
> + if (wvif->vif->bss_conf.assoc && wvif->vif->bss_conf.ps)
> + dev_info(wvif->wdev->dev, "ignoring requested
> PS mode");
> + return -1;
I can't be happy about this -1 return or how it's handled in the caller.
There is already a -1 return so it's not really a new bug, though...
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 1/1] binder: fix freeze race
On Thu, Sep 09, 2021 at 04:21:41PM -0700, Li Li wrote:
> @@ -4648,6 +4647,22 @@ static int binder_ioctl_get_node_debug_info(struct
> binder_proc *proc,
> return 0;
> }
>
> +static int binder_txns_pending(struct binder_proc *proc)
> +{
> + struct rb_node *n;
> + struct binder_thread *thread;
> +
> + if (proc->outstanding_txns > 0)
> + return 1;
Make this function bool.
> +
> + for (n = rb_first(>threads); n; n = rb_next(n)) {
> + thread = rb_entry(n, struct binder_thread, rb_node);
> + if (thread->transaction_stack)
> + return 1;
> + }
> + return 0;
> +}
> +
> static int binder_ioctl_freeze(struct binder_freeze_info *info,
> struct binder_proc *target_proc)
> {
> @@ -4682,6 +4697,14 @@ static int binder_ioctl_freeze(struct
> binder_freeze_info *info,
> if (!ret && target_proc->outstanding_txns)
> ret = -EAGAIN;
These two lines can be deleted now because binder_txns_pending() checks
->outstanding_txns.
>
> + /* Also check pending transactions that wait for reply */
> + if (ret >= 0) {
> + binder_inner_proc_lock(target_proc);
> + if (binder_txns_pending(target_proc))
> + ret = -EAGAIN;
> + binder_inner_proc_unlock(target_proc);
> + }
> +
> if (ret < 0) {
> binder_inner_proc_lock(target_proc);
> target_proc->is_frozen = false;
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binder: make sure fd closes complete
On Thu, Sep 02, 2021 at 08:35:35AM -0700, Todd Kjos wrote:
> On Tue, Aug 31, 2021 at 12:24 AM Martijn Coenen wrote:
> >
> > On Mon, Aug 30, 2021 at 9:51 PM 'Todd Kjos' via kernel-team
> > wrote:
> > >
> > > During BC_FREE_BUFFER processing, the BINDER_TYPE_FDA object
> > > cleanup may close 1 or more fds. The close operations are
> > > completed using the task work mechanism -- which means the thread
> > > needs to return to userspace or the file object may never be
> > > dereferenced -- which can lead to hung processes.
> > >
> > > Force the binder thread back to userspace if an fd is closed during
> > > BC_FREE_BUFFER handling.
> > >
> > > Signed-off-by: Todd Kjos
> > Reviewed-by: Martijn Coenen
>
> Please also add to stable releases 5.4 and later.
It would be better if this had a fixes tag so we knew which is the first
buggy commit.
There was a long Project Zero article about the Bad Binder exploit
because commit f5cb779ba163 ("ANDROID: binder: remove waitqueue when
thread exits.") was marked as # 4.14 but it didn't have a Fixes tag and
the actual buggy commit was in 4.9.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8723bs: fix memory leak error
On Tue, Aug 31, 2021 at 01:03:55AM +0530, F.A.Sulaiman wrote:
> Smatch reported memory leak bug in rtl8723b_FirmwareDownload function.
> The problem is pFirmware memory is not released in release_fw1.
> Instead of redirecting to release_fw1 we can turn it into exit
> and free the memory.
>
> Signed-off-by: F.A. SULAIMAN
> ---
> drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> index de8caa6cd418..b59c2aa3a9d8 100644
> --- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> +++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> @@ -436,7 +436,7 @@ s32 rtl8723b_FirmwareDownload(struct adapter *padapter,
> bool bUsedWoWLANFw)
> if (pFirmware->fw_length > FW_8723B_SIZE) {
> rtStatus = _FAIL;
> DBG_871X_LEVEL(_drv_emerg_, "Firmware size:%u exceed %u\n",
> pFirmware->fw_length, FW_8723B_SIZE);
> - goto release_fw1;
> + goto exit;
> }
The current tree doesn't have DBG_871X_LEVEL() so you must be working
against something old. You need to work against linux-next or staging
next.
Your patch fixes a bug, but it would be better to just re-write the
error handling for this function. There is another bug that a bunch
of error paths don't call release_firmware(fw). Use the "Free the Last
Thing" method.
pFirmware = kzalloc(sizeof(struct rt_firmware), GFP_KERNEL);
if (!pFirmware)
return _FAIL;
The last thing we allocated is "pFirmware" so free that if we have an
error.
pBTFirmware = kzalloc(sizeof(struct rt_firmware), GFP_KERNEL);
if (!pBTFirmware) {
rtStatus = _FAIL;
goto free_firmware;
}
Now the last thing is pBTFirmware.
rtStatus = request_firmware(, fwfilepath, device);
if (rtStatus) {
rtStatus = _FAIL;
goto free_bt_firmware;
}
Now the last thing is "fw". But this is a bit tricky because we're
going to release it as soon as possible and not wait until the end of
the function. There isn't a reason for this... We can change that or
keep it as-is. If we keep it as is, then the we'll just call
release_firmware(fw); before the goto free_bt_firmware; The current
code leaks fw on a bunch of error paths.
pFirmware->fw_buffer_sz = kmemdup(fw->data, fw->size, GFP_KERNEL);
if (!pFirmware->fw_buffer_sz) {
rtStatus = _FAIL;
release_firmware(fw);
goto free_bt_firmware;
}
Or:
pFirmware->fw_buffer_sz = kmemdup(fw->data, fw->size, GFP_KERNEL);
if (!pFirmware->fw_buffer_sz) {
rtStatus = _FAIL;
goto release_fw;
}
Now the last thing is pFirmware->fw_buffer_sz. Etc.
Then at the end it's just:
free_fw_buffer:
kfree(pFirmware->fw_buffer_sz);
release_fw:
release_firmware(fw);
free_bt_firmware:
kfree(pBTFirmware);
free_firmware:
kfree(pFirmware);
return rtStatus;
}
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging/ks7010: Fix coding style problems [Version 2]
Google for how to format a v2 patch. On Mon, Aug 16, 2021 at 08:04:47PM +0200, Leon Krieg wrote: > By doing some last-second wording changes directly in the diff I've > screwed up and managed to use spaces instead of tabs for the Kconfig file. > This is embarrassing! > I love adding backstory to my commit messages and but this is a bit much. :P Just say "Use tabs instead of spaces". Add the backstory under the --- cut off line if necessary. Also it doesn't just change the Kconfig file. There are a lot of unrelated changes as well. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 1/3] drm/loongson: Add DRM Driver for Loongson 7A1000 bridge chip
On Fri, Jul 23, 2021 at 11:12:49AM +0800, lichenyang wrote:
> +static int loongson_drm_load(struct drm_device *dev)
> +{
> + struct loongson_device *ldev;
> + int ret;
> +
> + ldev = devm_kzalloc(dev->dev, sizeof(*ldev), GFP_KERNEL);
> + if (!ldev)
> + return -ENOMEM;
> +
> + dev->dev_private = ldev;
> + ldev->dev = dev;
> +
> + ret = loongson_device_init(dev);
> + if (ret)
> + goto err;
> +
> + ret = drmm_vram_helper_init(dev, ldev->vram_start, ldev->vram_size);
> + if (ret) {
> + drm_err(dev, "Error initializing vram %d\n", ret);
> + goto err;
> + }
> +
> + drm_mode_config_init(dev);
> + dev->mode_config.funcs = (void *)_mode_funcs;
> + dev->mode_config.min_width = 1;
> + dev->mode_config.min_height = 1;
> + dev->mode_config.max_width = 4096;
> + dev->mode_config.max_height = 4096;
> + dev->mode_config.preferred_depth = 32;
> + dev->mode_config.prefer_shadow = 1;
> + dev->mode_config.fb_base = ldev->vram_start;
> +
> + ret = loongson_modeset_init(ldev);
> + if (ret) {
> + drm_err(dev, "Fatal error during modeset init: %d\n", ret);
> + goto err;
> + }
> +
> + drm_kms_helper_poll_init(dev);
> + drm_mode_config_reset(dev);
> +
> + return 0;
> +
> +err:
> + kfree(ldev);
I'm sorry, in the earlier version I told you to add this kfree() but
this is devm_ allocated so the kfree() is wrong and will lead to a
double free. My bad. That's on me.
> + drm_err(dev, "failed to initialize drm driver: %d\n", ret);
> + return ret;
> +}
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/3] drm/loongson: Add DRM Driver for Loongson 7A1000 bridge chip
On Thu, Jul 15, 2021 at 09:58:07AM +0800, lichenyang wrote:
> +int loongson_crtc_init(struct loongson_device *ldev, int index)
> +{
> + struct loongson_crtc *lcrtc;
> + u32 ret;
This should be "int ret;"
> +
> + lcrtc = kzalloc(sizeof(struct loongson_crtc), GFP_KERNEL);
> + if (lcrtc == NULL)
> + return -1;
> +
> + lcrtc->ldev = ldev;
> + lcrtc->reg_offset = index * REG_OFFSET;
> + lcrtc->cfg_reg = CFG_RESET;
> + lcrtc->crtc_id = index;
> +
> + ret = loongson_plane_init(lcrtc);
> + if (ret)
> + return ret;
> +
> + ret = drm_crtc_init_with_planes(ldev->dev, >base, lcrtc->plane,
> + NULL, _crtc_funcs, NULL);
> + if (ret) {
> + DRM_ERROR("failed to init crtc %d\n", index);
> + drm_plane_cleanup(lcrtc->plane);
> + return ret;
> + }
> +
> + drm_crtc_helper_add(>base, _crtc_helper_funcs);
> +
> + ldev->mode_info[index].crtc = lcrtc;
> +
> + return 0;
> +}
[ snip ]
> +int loongson_modeset_init(struct loongson_device *ldev)
> +{
> + struct drm_encoder *encoder;
> + struct drm_connector *connector;
> + int i;
> + u32 ret;
Same.
> +
> + ldev->dev->mode_config.allow_fb_modifiers = true;
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 3/3] drm/loongson: Add interrupt driver for LS7A
q_uninstall(struct drm_device *dev);
> +
> /* i2c */
> int loongson_dc_gpio_init(struct loongson_device *ldev);
>
> diff --git a/drivers/gpu/drm/loongson/loongson_irq.c
> b/drivers/gpu/drm/loongson/loongson_irq.c
> new file mode 100644
> index ..d212e16f3c00
> --- /dev/null
> +++ b/drivers/gpu/drm/loongson/loongson_irq.c
> @@ -0,0 +1,105 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include "loongson_drv.h"
> +#include
> +
> +int loongson_irq_init(struct loongson_device *ldev)
> +{
> + struct drm_device *dev;
> + int ret, irq;
> +
> + dev = ldev->dev;
> + irq = dev->pdev->irq;
> +
> + ret = drm_vblank_init(dev, ldev->num_crtc);
> + if (ret) {
> + dev_err(dev->dev, "Fatal error during vblank init: %d\n", ret);
> + return ret;
> + }
> + DRM_INFO("drm vblank init finished\n");
> +
> + ret = drm_irq_install(dev, irq);
> + if (ret) {
> + dev_err(dev->dev, "Fatal error during irq install: %d\n", ret);
> + return ret;
> + }
> + DRM_INFO("loongson irq initialized\n");
> +
> + return 0;
> +}
> +
> +int loongson_crtc_enable_vblank(struct drm_crtc *crtc)
> +{
> + struct loongson_crtc *lcrtc = to_loongson_crtc(crtc);
> + struct loongson_device *ldev = lcrtc->ldev;
> + u32 reg_val;
> +
> + if (lcrtc->crtc_id) {
> + reg_val = ls7a_mm_rreg(ldev, FB_INT_REG);
> + reg_val |= FB_VSYNC1_ENABLE;
> + ls7a_mm_wreg(ldev, FB_INT_REG, reg_val);
> + } else {
> + reg_val = ls7a_mm_rreg(ldev, FB_INT_REG);
> + reg_val |= FB_VSYNC0_ENABLE;
> + ls7a_mm_wreg(ldev, FB_INT_REG, reg_val);
> + }
> +
> + return 0;
> +}
> +
> +void loongson_crtc_disable_vblank(struct drm_crtc *crtc)
> +{
> + struct loongson_crtc *lcrtc = to_loongson_crtc(crtc);
> + struct loongson_device *ldev = lcrtc->ldev;
> + u32 reg_val;
> +
> + if (lcrtc->crtc_id) {
> + reg_val = ls7a_mm_rreg(ldev, FB_INT_REG);
> + reg_val &= ~FB_VSYNC1_ENABLE;
> + ls7a_mm_wreg(ldev, FB_INT_REG, reg_val);
> + } else {
> + reg_val = ls7a_mm_rreg(ldev, FB_INT_REG);
> + reg_val &= ~FB_VSYNC0_ENABLE;
> + ls7a_mm_wreg(ldev, FB_INT_REG, reg_val);
> + }
More readable to pull the common code in one place:
reg_val = ls7a_mm_rreg(ldev, FB_INT_REG);
if (lcrtc->crtc_id)
reg_val &= ~FB_VSYNC1_ENABLE;
else
reg_val &= ~FB_VSYNC0_ENABLE;
ls7a_mm_wreg(ldev, FB_INT_REG, reg_val);
> +}
> +
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/3] drm/loongson: Add DRM Driver for Loongson 7A1000 bridge chip
N_DC 0x7a06
> +#define PCI_DEVICE_ID_LOONGSON_GPU 0x7a15
> +#define LS7A_PIX_PLL (0x04b0)
> +#define REG_OFFSET (0x10)
> +#define FB_CFG_REG (0x1240)
> +#define FB_ADDR0_REG (0x1260)
> +#define FB_ADDR1_REG (0x1580)
> +#define FB_STRI_REG (0x1280)
> +#define FB_DITCFG_REG (0x1360)
> +#define FB_DITTAB_LO_REG (0x1380)
> +#define FB_DITTAB_HI_REG (0x13a0)
> +#define FB_PANCFG_REG (0x13c0)
> +#define FB_PANTIM_REG (0x13e0)
> +#define FB_HDISPLAY_REG (0x1400)
> +#define FB_HSYNC_REG (0x1420)
> +#define FB_VDISPLAY_REG (0x1480)
> +#define FB_VSYNC_REG (0x14a0)
> +
> +#define CFG_FMT GENMASK(2, 0)
> +#define CFG_FBSWITCH BIT(7)
> +#define CFG_ENABLE BIT(8)
> +#define CFG_FBNUM BIT(11)
> +#define CFG_GAMMAR BIT(12)
> +#define CFG_RESET BIT(20)
> +
> +#define FB_PANCFG_DEF 0x80001311
> +#define FB_HSYNC_PULSE (1 << 30)
> +#define FB_VSYNC_PULSE (1 << 30)
> +
> +/* PIX PLL */
> +#define LOOPC_MIN 24
> +#define LOOPC_MAX 161
> +#define FRE_REF_MIN 12
> +#define FRE_REF_MAX 32
> +#define DIV_REF_MIN 3
> +#define DIV_REF_MAX 5
> +#define PST_DIV_MAX 64
> +
> +struct pix_pll {
> + u32 l2_div;
> + u32 l1_loopc;
> + u32 l1_frefc;
> +};
> +
> +struct loongson_crtc {
> + struct drm_crtc base;
> + struct loongson_device *ldev;
> + u32 crtc_id;
> + u32 reg_offset;
> + u32 cfg_reg;
> + struct drm_plane *plane;
> +};
> +
> +struct loongson_encoder {
> + struct drm_encoder base;
> + struct loongson_device *ldev;
> + struct loongson_crtc *lcrtc;
> +};
> +
> +struct loongson_connector {
> + struct drm_connector base;
> + struct loongson_device *ldev;
> + u16 id;
> + u32 type;
> +};
> +
> +struct loongson_mode_info {
> + struct loongson_device *ldev;
> + struct loongson_crtc *crtc;
> + struct loongson_encoder *encoder;
> + struct loongson_connector *connector;
> +};
> +
> +struct loongson_device {
> + struct drm_device *dev;
> + struct drm_atomic_state *state;
> +
> + void __iomem *mmio;
> + void __iomem *io;
> + u32 vram_start;
> + u32 vram_size;
> +
> + u32 num_crtc;
> + struct loongson_mode_info mode_info[2];
> + struct pci_dev *gpu_pdev; /* LS7A gpu device info */
> +};
> +
> +/* crtc */
> +int loongson_crtc_init(struct loongson_device *ldev, int index);
> +
> +/* connector */
> +int loongson_connector_init(struct loongson_device *ldev, int index);
> +
> +/* encoder */
> +int loongson_encoder_init(struct loongson_device *ldev, int index);
> +
> +/* plane */
> +int loongson_plane_init(struct loongson_crtc *lcrtc);
> +
> +/* device */
> +u32 loongson_gpu_offset(struct drm_plane_state *state);
> +u32 ls7a_mm_rreg(struct loongson_device *ldev, u32 offset);
> +void ls7a_mm_wreg(struct loongson_device *ldev, u32 offset, u32 val);
> +u32 ls7a_io_rreg(struct loongson_device *ldev, u32 offset);
> +void ls7a_io_wreg(struct loongson_device *ldev, u32 offset, u32 val);
> +
> +#endif /* __LOONGSON_DRV_H__ */
> diff --git a/drivers/gpu/drm/loongson/loongson_encoder.c
> b/drivers/gpu/drm/loongson/loongson_encoder.c
> new file mode 100644
> index ..2002cee00303
> --- /dev/null
> +++ b/drivers/gpu/drm/loongson/loongson_encoder.c
> @@ -0,0 +1,37 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include "loongson_drv.h"
> +
> +static void loongson_encoder_destroy(struct drm_encoder *encoder)
> +{
> + struct loongson_encoder *lencoder = to_loongson_encoder(encoder);
> +
> + drm_encoder_cleanup(encoder);
> + kfree(lencoder);
> +}
> +
> +static const struct drm_encoder_funcs loongson_encoder_funcs = {
> + .destroy = loongson_encoder_destroy,
> +};
> +
> +int loongson_encoder_init(struct loongson_device *ldev, int index)
> +{
> + struct drm_encoder *encoder;
> + struct loongson_encoder *lencoder;
> +
> + lencoder = kzalloc(sizeof(struct loongson_encoder), GFP_KERNEL);
> + if (!lencoder)
> + return -1;
return -ENOMEM;
> +
> + lencoder->lcrtc = ldev->mode_info[index].crtc;
> + lencoder->ldev = ldev;
> + encoder = >base;
> + encoder->possible_crtcs = 1 << index;
> +
> + drm_encoder_init(ldev->dev, encoder, _encoder_funcs,
> + DRM_MODE_ENCODER_DAC, NULL);
> +
> + ldev->mode_info[index].encoder = lencoder;
> +
> + return 0;
> +}
> diff --git a/drivers/gpu/drm/loongson/loongson_plane.c
> b/drivers/gpu/drm/loongson/loongson_plane.c
> new file mode 100644
> index ..b8c247d1ce09
> --- /dev/null
> +++ b/drivers/g
Re: [PATCH v2 2/3] drm/loongson: Add GPIO and I2C driver for loongson drm.
lgo_data->udelay = DC_I2C_TON;
> + i2c_algo_data->timeout = usecs_to_jiffies(2200);
> +
> + ret = i2c_bit_add_numbered_bus(i2c_adapter);
> + if (ret) {
> + DRM_ERROR("Failed to register i2c algo-bit adapter %s\n",
> + i2c_adapter->name);
> + kfree(i2c_adapter);
> + i2c_adapter = NULL;
No need to assign this to NULL. It leaks algo_data;
> + }
if (ret)
goto free_algo_data;
> +
> + li2c->adapter = i2c_adapter;
> + i2c_algo_data->data = li2c;
> + i2c_set_adapdata(li2c->adapter, li2c);
> + DRM_INFO("Register i2c algo-bit adapter [%s]\n", i2c_adapter->name);
> +
> + memset(_info, 0, sizeof(struct i2c_board_info));
> + strncpy(i2c_info.type, name, I2C_NAME_SIZE);
> + i2c_info.addr = DDC_ADDR;
> + i2c_cli = i2c_new_client_device(i2c_adapter, _info);
> + if (i2c_cli == NULL) {
Write these as:
if (!i2c_cli)
But actually i2c_new_client_device() returns error pointers.
if (IS_ERR(i2c_cli)) {
ret = PTR_ERR(i2c_cli);
> + DRM_ERROR("Failed to create i2c adapter\n");
> + return -EBUSY;
Leaks stuff.
goto remove_numbered_bus?
> + }
> + li2c->init = true;
> + return 0;
> +
> +error_mem:
> + DRM_ERROR("Failed to malloc memory for loongson i2c\n");
> + return ret;
remove_numbered_bus:
do something()?
free_algo_data:
kfree(i2c_algo_data);
free_adapter:
kfree(i2c_adapter);
return ret;
> +}
> +
> +static int loongson_i2c_add(struct loongson_device *ldev, const char *name)
> +{
> + int i;
> +
> + for (i = 0; i < LS_MAX_I2C_BUS; i++) {
> + if (ldev->i2c_bus[i].use)
Flip this around. Always do error handling instead of success handling.
if (!ldev->i2c_bus[i].use) {
DRM_DEBUG_DRIVER("i2c_bus[%d] not use\n", i);
return -ENODEV;
}
> + loongson_i2c_create(>i2c_bus[i], name);
Pull this code back a tab. Check for errors.
ret = loongson_i2c_create(>i2c_bus[i], name);
if (ret)
return ret;
> + else {
> + DRM_DEBUG_DRIVER("i2c_bus[%d] not use\n", i);
> + return -ENODEV;
> + }
> + }
> + return 0;
> +}
> +
> +int loongson_dc_gpio_init(struct loongson_device *ldev)
> +{
> + int ret;
> + struct gpio_chip *chip;
struct gpio_chip *chip = >chip;
int ret;
> +
> + chip = >chip;
> + chip->label = "ls7a-dc-gpio";
> + chip->base = LS7A_DC_GPIO_BASE;
> + chip->ngpio = 4;
> + chip->parent = ldev->dev->dev;
> + chip->request = ls_dc_gpio_request;
> + chip->direction_input = ls_dc_gpio_dir_input;
> + chip->direction_output = ls_dc_gpio_dir_output;
> + chip->set = ls_dc_gpio_set;
> + chip->get = ls_dc_gpio_get;
> + chip->can_sleep = false;
> +
> + ret = devm_gpiochip_add_data(ldev->dev->dev, chip, ldev);
> + if (ret) {
> + DRM_ERROR("Failed to register ls7a dc gpio driver\n");
> + return -ENODEV;
return ret;
> + }
> + DRM_INFO("Registered ls7a dc gpio driver\n");
> +
> + return 0;
> +}
> +
> +int loongson_i2c_init(struct loongson_device *ldev)
> +{
> + int ret;
> +
> + ret = gpio_request_array(i2c_gpios, ARRAY_SIZE(i2c_gpios));
> + if (ret) {
> + DRM_ERROR("Failed to request gpio array i2c_gpios\n");
> + return -ENODEV;
return ret;
> + }
> +
> + ldev->i2c_bus[0].i2c_id = 6;
> + ldev->i2c_bus[0].use = true;
> + ldev->i2c_bus[1].i2c_id = 7;
> + ldev->i2c_bus[1].use = true;
> +
> + loongson_i2c_add(ldev, DC_I2C_NAME);
check for errors.
> +
> + return 0;
> +}
> +
> +struct loongson_i2c *loongson_i2c_bus_match(struct loongson_device *ldev,
> u32 i2c_id)
> +{
> + u32 i;
> + struct loongson_i2c *match = NULL, *tables;
> +
> + tables = ldev->i2c_bus;
> +
> + for (i = 0; i < LS_MAX_I2C_BUS; i++) {
table = >i2c_bus[i];
> + if (tables->i2c_id == i2c_id && tables->init == true) {
> + match = tables;
> + break;
Just return instead of break:
return table;
> + }
> +
> + tables++;
> + }
> +
> + return match;
> +}
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/3] drm/loongson: Add interrupt driver for LS7A
On Tue, Jul 06, 2021 at 02:36:31PM +0800, lichenyang wrote: > int loongson_crtc_init(struct loongson_device *ldev, int index) > diff --git a/drivers/gpu/drm/loongson/loongson_drv.c > b/drivers/gpu/drm/loongson/loongson_drv.c > index 252be9e25aff..89450c8c9102 100644 > --- a/drivers/gpu/drm/loongson/loongson_drv.c > +++ b/drivers/gpu/drm/loongson/loongson_drv.c > @@ -167,6 +167,10 @@ static int loongson_drm_load(struct drm_device *dev, > unsigned long flags) > if (ret) > dev_err(dev->dev, "Fatal error during modeset init: %d\n", ret); > > + ret = loongson_irq_init(ldev); > + if (ret) > + dev_err(dev->dev, "Fatal error during irq init: %d\n", ret); It feel like there should be proper cleanup and error handling on this path instead of just printing an error and continuing. > + > drm_kms_helper_poll_init(dev); > drm_mode_config_reset(dev); > regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Fixed a coding style issue - missing a blank line after declarations
On Sun, May 23, 2021 at 10:21:51AM +0900, Donggyu Kim wrote: > Signed-off-by:Donggyu Kim The subject needs a subsystem prefix, the subject is too long, and there is no commit message. Run scripts/checkpatch.pl on your patches. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/5] staging: media: atomisp: code formatting changes sh_css_params.c
On Sat, May 01, 2021 at 12:16:07PM +0530, Deepak R Varma wrote: > @@ -1562,8 +1544,10 @@ ia_css_isp_3a_statistics_map_allocate( > base_ptr = me->data_ptr; > > me->size = isp_stats->size; > - /* GCC complains when we assign a char * to a void *, so these > - * casts are necessary unfortunately. */ > + /* > + * GCC complains when we assign a char * to a void *, so these > + * casts are necessary unfortunately. > + */ Not related to your patch, but assigning a char pointer to a void pointer is fine and GCC will not complain. The problem is that me->dmem_stats is not a void pointer. Someone should delete that nonsense comment. > me->dmem_stats= (void *)base_ptr; > me->vmem_stats_hi = (void *)(base_ptr + isp_stats->dmem_size); > me->vmem_stats_lo = (void *)(base_ptr + isp_stats->dmem_size + regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: iio: cdc: ad7746: Remove unnecessary assignment in ad7746_probe()
On Tue, May 18, 2021 at 05:56:47PM +0800, Tang Bin wrote: > In the function ad7746_probe(), the initialized value of 'ret' is unused, > because it will be assigned by the function i2c_smbus_write_byte_data(), > thus remove it. > > Signed-off-by: Tang Bin Thanks! Reviewed-by: Dan Carpenter regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: iio: cdc: ad7746: Fix unnecessary check and assignment in ad7746_probe()
On Mon, May 17, 2021 at 11:00:06PM +0800, Tang Bin wrote: > @@ -730,11 +730,7 @@ static int ad7746_probe(struct i2c_client *client, > if (ret < 0) > return ret; > > - ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev); > - if (ret) > - return ret; > - > - return 0; > + return devm_iio_device_register(indio_dev->dev.parent, indio_dev); > } This sort of thing is done deliberately as a style choice... I probably wouldn't have written it that way myself, but there really isn't a downside to leaving it as-is. The unused "int ret = 0;" just introduces a static checker warning about unused assignments and disables the static checker warning for uninitialized variables so we want to remove that. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 07/17] staging: nuc-wmi: add basic support for NUC6 WMI
On Sun, May 16, 2021 at 12:53:35PM +0200, Mauro Carvalho Chehab wrote:
> +static int nuc_wmi_query_leds_nuc6(struct device *dev)
> +{
> + // FIXME: add a check for the specific models that are known to work
> + struct nuc_wmi *priv = dev_get_drvdata(dev);
> + u8 cmd, input[NUM_INPUT_ARGS] = { 0 };
> + u8 output[NUM_OUTPUT_ARGS];
> + struct nuc_nmi_led *led;
> + int ret;
> +
> + cmd = LED_OLD_GET_STATUS;
> + input[0] = LED_OLD_GET_S0_POWER;
> + ret = nuc_nmi_cmd(dev, cmd, input, output);
> + if (ret) {
> + dev_warn(dev, "Get S0 Power: error %d\n", ret);
> + return ret;
> + }
> +
> + led = >led[priv->num_leds];
> + led->id = POWER_LED;
> + led->color_type = LED_BLUE_AMBER;
> + led->avail_indicators = LED_IND_POWER_STATE;
> + led->indicator = fls(led->avail_indicators);
> + priv->num_leds++;
> +
> + cmd = LED_OLD_GET_STATUS;
> + input[0] = LED_OLD_GET_S0_RING;
> + ret = nuc_nmi_cmd(dev, cmd, input, output);
> + if (ret) {
> + dev_warn(dev, "Get S0 Ring: error %d\n", ret);
> + return ret;
> + }
> + led = >led[priv->num_leds];
> + led->id = RING_LED;
> + led->color_type = LED_BLUE_AMBER;
> + led->avail_indicators = LED_IND_SOFTWARE;
> + led->indicator = fls(led->avail_indicators);
> + priv->num_leds++;
> +
> + return ret;
return 0;
> +}
> +
> static int nuc_wmi_query_leds(struct device *dev, enum led_api_rev *api_rev)
> {
> struct nuc_wmi *priv = dev_get_drvdata(dev);
> u8 input[NUM_INPUT_ARGS] = { 0 };
> u8 output[NUM_OUTPUT_ARGS];
> - int id, ret, ver = LED_API_UNKNOWN;
> + int id, ret, ver = LED_API_UNKNOWN, nuc_ver = 0;
> u8 leds;
> + const char *dmi_name;
> +
> + dmi_name = dmi_get_system_info(DMI_PRODUCT_NAME);
> + if (!dmi_name || !*dmi_name)
> + dmi_name = dmi_get_system_info(DMI_BOARD_NAME);
> +
> + if (strncmp(dmi_name, "NUC", 3))
> + return -ENODEV;
> +
> + dmi_name +=3;
> + while (*dmi_name) {
> + if (*dmi_name < '0' || *dmi_name > '9')
> + break;
> + nuc_ver = (*dmi_name - '0') + nuc_ver * 10;
> + dmi_name++;
> + }
> +
> + if (nuc_ver < 6)
> + return -ENODEV;
> +
> + if (nuc_ver < 8) {
> + *api_rev = LED_API_NUC6;
> + return nuc_wmi_query_leds_nuc6(dev);
> + }
>
> - /*
> - * List all LED types support in the platform
> - *
> - * Should work with both NUC8iXXX and NUC10iXXX
> - *
> - * FIXME: Should add a fallback code for it to work with older NUCs,
> - * as LED_QUERY returns an error on older devices like Skull Canyon.
> - */
> input[0] = LED_QUERY_LIST_ALL;
> ret = nuc_nmi_cmd(dev, LED_QUERY, input, output);
> - if (ret == -ENOENT) {
> - ver = LED_API_NUC6;
> - } else if (ret) {
> + if (ret) {
> dev_warn(dev, "error %d while listing all LEDs\n", ret);
> return ret;
> } else {
> leds = output[0];
> }
Delete the else and pull the assignment in a tab.
>
> - if (ver != LED_API_NUC6) {
> - ret = nuc_nmi_cmd(dev, LED_VERSION_CONTROL, input, output);
> - ver = output[0] | output[1] << 16;
> - if (!ver)
> - ver = LED_API_REV_0_64;
> - else if (ver == 0x0126)
> - ver = LED_API_REV_1_0;
> - }
> + ret = nuc_nmi_cmd(dev, LED_VERSION_CONTROL, input, output);
> + ver = output[0] | output[1] << 16;
> + if (!ver)
> + *api_rev = LED_API_REV_0_64;
> + else if (ver == 0x0126)
> + *api_rev = LED_API_REV_1_0;
>
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 02/17] staging: nuc-wmi: detect WMI API detection
On Sun, May 16, 2021 at 12:53:30PM +0200, Mauro Carvalho Chehab wrote:
> - leds = output[0];
> + if (ver != LED_API_NUC6) {
> + ret = nuc_nmi_cmd(dev, LED_VERSION_CONTROL, input, output);
Does this need to be checked?
if (ret)
return ret;
> + ver = output[0] | output[1] << 16;
> + if (!ver)
> + ver = LED_API_REV_0_64;
> + else if (ver == 0x0126)
> + ver = LED_API_REV_1_0;
> + }
> +
> + /* Currently, only API Revision 0.64 is supported */
> + if (ver != LED_API_REV_0_64)
> + return -ENODEV;
> +
> if (!leds) {
> dev_warn(dev, "No LEDs found\n");
> return -ENODEV;
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8192e: fix array of flexible structures
On Tue, Apr 27, 2021 at 11:19:45PM +0530, Jitendra Khasdev wrote:
> This patch fixes sparse warning "array of flexible structures"
> for rtllib.h.
>
> eg. drivers/staging/rtl8192e/rtllib.h:832:48: warning: array of
> flexible structures
>
> Signed-off-by: Jitendra Khasdev
> ---
> drivers/staging/rtl8192e/rtllib.h | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/rtl8192e/rtllib.h
> b/drivers/staging/rtl8192e/rtllib.h
> index 4cabaf2..c7cb318 100644
> --- a/drivers/staging/rtl8192e/rtllib.h
> +++ b/drivers/staging/rtl8192e/rtllib.h
> @@ -802,7 +802,7 @@ struct rtllib_authentication {
> __le16 transaction;
> __le16 status;
> /*challenge*/
> - struct rtllib_info_element info_element[];
> + struct rtllib_info_element *info_element;
> } __packed;
This patch is wrong.
The original code is basically fine. Normally it doesn't make sense to
have an array of flex arrays, but in this case it "flexes" between 0 and
1. If it were had two elements then the match the math wouldn't work
at all.
We should probably get rid of it and just add some giant comments and
defines to do the math.
But changing it to a pointer isn't right.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/78] media: use pm_runtime_resume_and_get() instead of pm_runtime_get_sync()
There was a Smatch check for these bugs. This was a good source of recurring Reported-by tags for me. ;) Thanks for doing this. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] media: atomisp: silence "dubious: !x | !y" warning
On Sat, Apr 17, 2021 at 09:31:32PM +, David Laight wrote: > From: Mauro Carvalho Chehab > > Sent: 17 April 2021 19:56 > > > > Em Sat, 17 Apr 2021 21:06:27 +0530 > > Ashish Kalra escreveu: > > > > > Upon running sparse, "warning: dubious: !x | !y" is brought to notice > > > for this file. Logical and bitwise OR are basically the same in this > > > context so it doesn't cause a runtime bug. But let's change it to > > > logical OR to make it cleaner and silence the Sparse warning. > > The old code is very likely to by slightly more efficient. > > It may not matter here, but it might in a really hot path. > > Since !x | !y and !x || !y always have the same value > why is sparse complaining at all. > Smatch doesn't warn about | vs || if both sides are true/false. But I've occasionally asked people if they were trying to do a fast path optimization but it's always just a typo. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: greybus: fix fw is NULL but dereferenced
The commit description is not clear but this patch doesn't change how the code works, it just silences a static checker false positive. Just ignore the false positive. Always just ignore static checkers when they are wrong. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 02/11] staging: rtl8723bs: moved function prototypes out of core/rtw_efuse.c
On Mon, Mar 22, 2021 at 03:31:40PM +0100, Fabio Aiuto wrote: > fix the following checkpatch issues: > > WARNING: externs should be avoided in .c files > 35: FILE: drivers/staging/rtl8723bs/core/rtw_efuse.c:35: > +bool > > moved two function prototypes in include/rtw_efuse.h Can't you just make these functions static instead? regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 12/15] staging: rtl8723bs: remove unnecessary logging in os_dep/ioctl_cfg80211.c
On Thu, Mar 18, 2021 at 04:26:07PM +0100, Fabio Aiuto wrote:
> @@ -1405,7 +1398,6 @@ void rtw_cfg80211_surveydone_event_callback(struct
> adapter *padapter)
> struct wlan_network*pnetwork = NULL;
>
> #ifdef DEBUG_CFG80211
> - DBG_8192C("%s\n", __func__);
> #endif
The #ifdef can go as well.
>
> spin_lock_bh(&(pmlmepriv->scanned_queue.lock));
regads,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/43] Staging: rtl8723bs: fix names in rtw_mlme.h
On Wed, Mar 17, 2021 at 11:20:48PM +0100, Marco Cesati wrote:
> diff --git a/drivers/staging/rtl8723bs/include/rtw_mlme.h
> b/drivers/staging/rtl8723bs/include/rtw_mlme.h
> index 1ebc1e183381..ffcceb1fdde6 100644
> --- a/drivers/staging/rtl8723bs/include/rtw_mlme.h
> +++ b/drivers/staging/rtl8723bs/include/rtw_mlme.h
> @@ -81,13 +81,13 @@ enum dot11AuthAlgrthmNum {
> };
>
> /* Scan type including active and passive scan. */
> -enum RT_SCAN_TYPE {
> +enum rt_scan_type {
> SCAN_PASSIVE,
> SCAN_ACTIVE,
> SCAN_MIX,
> };
>
> -enum _BAND {
> +enum _band {
_band is a bad name.
> GHZ24_50 = 0,
> GHZ_50,
> GHZ_24,
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: staging: qlge: Fixed an alignment issue.
On Tue, Mar 16, 2021 at 09:26:34PM +0530, Anish Udupa wrote: > The * of the comment was not aligned properly. Ran checkpatch and > found the warning. Resolved it in this patch. > > Signed-off-by: Anish Udupa > --- > drivers/staging/qlge/qlge_main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/qlge/qlge_main.c > b/drivers/staging/qlge/qlge_main.c > index 5516be3af898..bfd7217f3953 100644 > --- a/drivers/staging/qlge/qlge_main.c > +++ b/drivers/staging/qlge/qlge_main.c > @@ -3816,7 +3816,7 @@ static int qlge_adapter_down(struct qlge_adapter *qdev) > qlge_tx_ring_clean(qdev); > > /* Call netif_napi_del() from common point. > - */ > + */ This has already been fixed upstream. You should be working against linux-next or staging-next. https://lore.kernel.org/driverdev-devel/20210216101945.187474-1-duche...@gmail.com/ regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: das800: fix request_irq() warn
On Wed, Mar 17, 2021 at 01:55:40AM -0400, Tong Zhang wrote: > Thanks for pointing that out. > Yes you are right there is a mistake. > I am a human. Human make mistakes. Therefore I make mistakes. > Yep. We all make mistakes. One thing to do is if you make a mistake then check to see if anyone else has made a similar mistake. git grep repalce If enough people make that specific mistake then consider adding it to the list of commonly mispelled words: scripts/spelling.txt I looked through the logs and it looks like someone mispells it once a year so it's probably not common enough to worry about. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: das800: fix request_irq() warn
On Tue, Mar 16, 2021 at 06:42:26PM -0400, Tong Zhang wrote: > request_irq() wont accept a name which contains slash so we need to > repalce it with something else -- otherwise it will trigger a warning ^^^ I don't normally comment on spelling mistakes in the commit message but you're copy and pasting "repalce" over and over... > and the entry in /proc/irq/ will not be created > since the .name might be used by userspace and we don't want to break > userspace, so we are changing the parameters passed to request_irq() regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/57] Staging: rtl8723bs: fix POINTER_LOCATION whitespaces
On Mon, Mar 15, 2021 at 06:05:21PM +0100, Marco Cesati wrote: > This set of patches fixes 522 checkpatch.pl errors of type > POINTER_LOCATION in the staging/rtl8723bs souce code. Every patch is > purely syntactical: it does not change the generated machine code. > Furthermore, every single patch leaves the source code fully compilable, > so that 'git bisect' will not be affected. Hopefully that part can be assumed. :P > > The checkpatch.pl script emits many errors and warnings for these > patches, however all of them are caused by the original code. They shall > be fixed in different patchsets. Yep. You maybe went a tad too aggressive in fixing the checkpatch warnings about parentheses... If you didn't introduce the warning, then you can just leave it as-is. Anyway, 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 v2] staging: rtl8192u: remove extra lines
On Tue, Mar 16, 2021 at 10:44:10AM +0800, zhaoxiao wrote: > Remove extra lines in many functions in r8192U_wx.c. > > Signed-off-by: zhaoxiao > --- > drivers/staging/rtl8192u/r8192U_wx.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) The commit message says you're removing lines but you're also adding them. :P regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 02/33] staging: rtl8723bs: remove typedefs in rtw_mlme.h
On Fri, Mar 12, 2021 at 09:26:07AM +0100, Marco Cesati wrote:
> diff --git a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
> b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
> index 1567831caf91..ed6b03c25367 100644
> --- a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
> +++ b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
> @@ -419,7 +419,7 @@ struct mlme_ext_info {
> /* The channel information about this channel including joining, scanning,
> and power constraints. */
> typedef struct _RT_CHANNEL_INFO {
> u8 ChannelNum; /* The channel number. */
> - RT_SCAN_TYPEScanType; /* Scan type such as passive
> or active scan. */
> + enum RT_SCAN_TYPE ScanType; /* Scan type such as
> passive or active scan. */
Originally ChannelNum and ScanType were aligned but now the indenting is
whacky. I think you did these patches with a script which is fine, but
always take a look over the over finished patch to double check by hand.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/33] staging: rtl8723bs: remove typedefs in HalBtcOutSrc.h
On Fri, Mar 12, 2021 at 09:26:06AM +0100, Marco Cesati wrote: > diff --git a/drivers/staging/rtl8723bs/hal/HalBtc8723b1Ant.c > b/drivers/staging/rtl8723bs/hal/HalBtc8723b1Ant.c > index ef8c6a0f31ae..87dc63408133 100644 > --- a/drivers/staging/rtl8723bs/hal/HalBtc8723b1Ant.c > +++ b/drivers/staging/rtl8723bs/hal/HalBtc8723b1Ant.c > @@ -151,7 +151,7 @@ static u8 halbtc8723b1ant_BtRssiState( > } > > static void halbtc8723b1ant_UpdateRaMask( > - PBTC_COEXIST pBtCoexist, bool bForceExec, u32 disRateMask > + struct BTC_COEXIST * pBtCoexist, bool bForceExec, u32 disRateMask There is an extra space between the "* pBtCoexist" which checkpatch warned you about. :/ It makes me sad that you did all this work without looking at the checkpatch output. ERROR: "foo * bar" should be "foo *bar" #146: FILE: drivers/staging/rtl8723bs/hal/HalBtc8723b1Ant.c:154: + struct BTC_COEXIST * pBtCoexist, bool bForceExec, u32 disRateMask regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8723bs: add initial value
On Thu, Mar 11, 2021 at 02:38:38PM +0800, Hao Peng wrote:
> Add initial value for some uninitialized variable and array.
>
None of these are ever used uninitialized. It's weird that you would
even think that.
> if (pmlmeext->active_keep_alive_check) {
> - int stainfo_offset;
> + int stainfo_offset = 0;
>
> stainfo_offset = rtw_stainfo_offset(pstapriv,
> psta);
This one is initialized on the very next line so all the patch does is
introduce static checker warnings for no reason.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8723bs: align comments
On Wed, Mar 10, 2021 at 04:37:21PM +0100, Fabio Aiuto wrote: > fix the following checkpatch warnings: > > WARNING: Block comments use * on subsequent lines > + /* > + AMPDU_para [1:0]:Max AMPDU Len => 0:8k , 1:16k, 2:32k, 3:64k > -- > WARNING: Block comments use * on subsequent lines > +/* > +op_mode > > Signed-off-by: Fabio Aiuto > --- > drivers/staging/rtl8723bs/core/rtw_ap.c | 28 - > 1 file changed, 14 insertions(+), 14 deletions(-) > > diff --git a/drivers/staging/rtl8723bs/core/rtw_ap.c > b/drivers/staging/rtl8723bs/core/rtw_ap.c > index b6f944b37b08..3a0e4f64466a 100644 > --- a/drivers/staging/rtl8723bs/core/rtw_ap.c > +++ b/drivers/staging/rtl8723bs/core/rtw_ap.c > @@ -721,9 +721,9 @@ static void update_hw_ht_param(struct adapter *padapter) > > /* handle A-MPDU parameter field */ > /* Combine these two comments into one mult-line comment. > - AMPDU_para [1:0]:Max AMPDU Len => 0:8k , 1:16k, 2:32k, 3:64k > - AMPDU_para [4:2]:Min MPDU Start Spacing > - */ > + * AMPDU_para [1:0]:Max AMPDU Len => 0:8k , 1:16k, 2:32k, 3:64k > + * AMPDU_para [4:2]:Min MPDU Start Spacing > + */ > max_AMPDU_len = pmlmeinfo->HT_caps.u.HT_cap_element.AMPDU_para & 0x03; > > min_MPDU_spacing = ( > @@ -1815,17 +1815,17 @@ void update_beacon(struct adapter *padapter, u8 > ie_id, u8 *oui, u8 tx) > } > > /* > -op_mode > -Set to 0 (HT pure) under the following conditions > - - all STAs in the BSS are 20/40 MHz HT in 20/40 MHz BSS or > - - all STAs in the BSS are 20 MHz HT in 20 MHz BSS > -Set to 1 (HT non-member protection) if there may be non-HT STAs > - in both the primary and the secondary channel > -Set to 2 if only HT STAs are associated in BSS, > - however and at least one 20 MHz HT STA is associated > -Set to 3 (HT mixed mode) when one or more non-HT STAs are associated > - (currently non-GF HT station is considered as non-HT STA also) > -*/ > + *op_mode You need to have a space character after the '*'. /* * op_mode * Set to ... > + *Set to 0 (HT pure) under the following conditions > + *- all STAs in the BSS are 20/40 MHz HT in 20/40 MHz BSS or > + *- all STAs in the BSS are 20 MHz HT in 20 MHz BSS > + *Set to 1 (HT non-member protection) if there may be non-HT STAs > + *in both the primary and the secondary channel > + *Set to 2 if only HT STAs are associated in BSS, > + *however and at least one 20 MHz HT STA is associated > + *Set to 3 (HT mixed mode) when one or more non-HT STAs are associated > + *(currently non-GF HT station is considered as non-HT STA also) > + */ regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[kbuild] Re: [PATCH v3 1/2] char: xillybus: Move class-related functions to new xillybus_class.c
url:
https://github.com/0day-ci/linux/commits/eli-billauer-gmail-com/Submission-of-XillyUSB-driver/20210309-193645
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git
080951f99de1e483a9a48f34c079b634f2912a54
config: x86_64-randconfig-m001-20210309 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
Reported-by: Dan Carpenter
smatch warnings:
drivers/char/xillybus/xillybus_class.c:86 xillybus_init_chrdev() warn: ignoring
unreachable code.
drivers/char/xillybus/xillybus_class.c:96 xillybus_init_chrdev() warn: missing
error code 'rc'
vim +86 drivers/char/xillybus/xillybus_class.c
10702b71f93292 Eli Billauer 2021-03-09 42 int xillybus_init_chrdev(struct
device *dev,
10702b71f93292 Eli Billauer 2021-03-09 43 const struct
file_operations *fops,
10702b71f93292 Eli Billauer 2021-03-09 44 struct module
*owner,
10702b71f93292 Eli Billauer 2021-03-09 45 void
*private_data,
10702b71f93292 Eli Billauer 2021-03-09 46 unsigned char
*idt, unsigned int len,
10702b71f93292 Eli Billauer 2021-03-09 47 int num_nodes,
10702b71f93292 Eli Billauer 2021-03-09 48 const char
*prefix, bool enumerate)
10702b71f93292 Eli Billauer 2021-03-09 49 {
10702b71f93292 Eli Billauer 2021-03-09 50 int rc;
10702b71f93292 Eli Billauer 2021-03-09 51 dev_t mdev;
10702b71f93292 Eli Billauer 2021-03-09 52 int i;
10702b71f93292 Eli Billauer 2021-03-09 53 char devname[48];
10702b71f93292 Eli Billauer 2021-03-09 54
10702b71f93292 Eli Billauer 2021-03-09 55 struct device *device;
10702b71f93292 Eli Billauer 2021-03-09 56 size_t namelen;
10702b71f93292 Eli Billauer 2021-03-09 57 struct xilly_unit *unit, *u;
10702b71f93292 Eli Billauer 2021-03-09 58
10702b71f93292 Eli Billauer 2021-03-09 59 unit = kzalloc(sizeof(*unit),
GFP_KERNEL);
10702b71f93292 Eli Billauer 2021-03-09 60
10702b71f93292 Eli Billauer 2021-03-09 61 if (!unit)
10702b71f93292 Eli Billauer 2021-03-09 62 return -ENOMEM;
10702b71f93292 Eli Billauer 2021-03-09 63
10702b71f93292 Eli Billauer 2021-03-09 64 mutex_lock(_mutex);
10702b71f93292 Eli Billauer 2021-03-09 65
10702b71f93292 Eli Billauer 2021-03-09 66 if (!enumerate)
10702b71f93292 Eli Billauer 2021-03-09 67 snprintf(unit->name,
UNITNAMELEN, "%s", prefix);
10702b71f93292 Eli Billauer 2021-03-09 68
10702b71f93292 Eli Billauer 2021-03-09 69 for (i = 0; enumerate; i++) {
10702b71f93292 Eli Billauer 2021-03-09 70 snprintf(unit->name,
UNITNAMELEN, "%s_%02d",
10702b71f93292 Eli Billauer 2021-03-09 71 prefix, i);
10702b71f93292 Eli Billauer 2021-03-09 72
10702b71f93292 Eli Billauer 2021-03-09 73 enumerate = false;
10702b71f93292 Eli Billauer 2021-03-09 74 list_for_each_entry(u,
_list, list_entry)
10702b71f93292 Eli Billauer 2021-03-09 75 if
(!strcmp(unit->name, u->name)) {
10702b71f93292 Eli Billauer 2021-03-09 76
enumerate = true;
10702b71f93292 Eli Billauer 2021-03-09 77 break;
10702b71f93292 Eli Billauer 2021-03-09 78 }
10702b71f93292 Eli Billauer 2021-03-09 79 }
10702b71f93292 Eli Billauer 2021-03-09 80
10702b71f93292 Eli Billauer 2021-03-09 81 rc = alloc_chrdev_region(,
0, num_nodes, unit->name);
10702b71f93292 Eli Billauer 2021-03-09 82
10702b71f93292 Eli Billauer 2021-03-09 83 if (rc) {
10702b71f93292 Eli Billauer 2021-03-09 84 dev_warn(dev, "Failed
to obtain major/minors");
10702b71f93292 Eli Billauer 2021-03-09 85 goto fail_obtain;
^
10702b71f93292 Eli Billauer 2021-03-09 @86 return rc;
^^
Unreachable
10702b71f93292 Eli Billauer 2021-03-09 87 }
10702b71f93292 Eli Billauer 2021-03-09 88
10702b71f93292 Eli Billauer 2021-03-09 89 unit->major = MAJOR(mdev);
10702b71f93292 Eli Billauer 2021-03-09 90 unit->lowest_minor =
MINOR(mdev);
10702b71f93292 Eli Billauer 2021-03-09 91 unit->num_nodes = num_nodes;
10702b71f93292 Eli Billauer 2021-03-09 92 unit->private_data =
private_data;
10702b71f93292 Eli Billauer 2021-03-09 93
10702b71f93292 Eli Billauer 2021-03-09 94 unit->cdev = cdev_alloc();
10702b71f93292 Eli Billauer 2021-03-09 95 if (!unit->cdev)
10702b71f93292 Eli Billauer 2021-03-09 @96 goto unregister_chrdev;
"rc = -ENOMEM;"
10702b71f93292 Eli Billauer 2021-03-09 97
10702b71f93292 Eli Billauer 2021-03-09 98 uni
Re: [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan
On Fri, Mar 05, 2021 at 03:00:14PM +, Lee wrote:
>
> Hi Dan,
>
> Do you think any of these could be potential issues:
>
> driver/staging/
>
> rtl8192e/rtllib_rx.c:2442
memcpy(dst->ssid, src->ssid, src->ssid_len);
Smatch says that at this point we know "src->ssid_len" is in the 1-32
range. This is without any fixes to how Smatch parses nl_len().
> wlan-ng/cfg80211.c:316
313 if (request->n_ssids > 0) {
314 msg1.scantype.data = P80211ENUM_scantype_active;
315 msg1.ssid.data.len = request->ssids->ssid_len;
316 memcpy(msg1.ssid.data.data,
317 request->ssids->ssid, request->ssids->ssid_len);
318 } else {
The only thing Smatch knows about "request->ssids->ssid_len" is that
it's 0-255. I had not marked "msg1.ssid.data.data" as a protected
struct member so it didn't generate a warning.
I think cfg80211_scan_request structs are filled out in a systematic
way in ieee80211_request_ibss_scan() and they're bounds checked properly
so this isn't a bug.
> rtl8723bs/os_dep/ioctl_cfg80211.c:1591
> rtl8723bs/os_dep/ioctl_cfg80211.c:2738
Same.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8188eu: prevent ->ssid overflow in rtw_wx_set_scan()
On Fri, Mar 05, 2021 at 10:58:17AM -0600, Edmundo Carmona Antoranz wrote:
> On Fri, Mar 5, 2021 at 2:59 AM Dan Carpenter wrote:
> > - if (sec_len > 0 && sec_len <= len) {
> > + if (sec_len > 0 &&
> > + sec_len <= len &&
> > + sec_len <= 32) {
>
> I wonder if this could be reduced to (sec_len > 0 && sec_len <=
> min(len, 32)) from a stylistic POV?
I kind of prefer it the way I wrote it. I prefer conditions split
apart and done ploddingly, one at a time... You'll notice how I could
have written it like:
if (sec_len > 0 && sec_len <= len &&
sec_len <= 32) {
But I really like my conditions to be spelled out so the "sec_len" is
perfectly aligned in each part of the condition. Your way would be to
combine two conditions into one part of a line and seems sneaky.
>
> First attempt at something kernel related so I know there's plenty of
> things to learn (including patterns for problems like this and
> etiquette).
It's good that you're reviewing code... We try to be predictable though
and no one would have predicted your response. Ideally patch review
should be like, "Ugh! Why didn't I think of that? Of course, we should
propagate the error code." Or "Oh, I didn't know checkpatch warns about
that."
The truth is that I don't always agree with all of Greg's reviews. He
is more strict than I am about breaking up patches into multiple things.
(It's a tricky line to define for me). But I can always predict what
Greg will say in a review so that saves time when I know which patches
he will accept and which he won't.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: rtl8188eu: prevent ->ssid overflow in rtw_wx_set_scan()
This code has a check to prevent read overflow but it needs another
check to prevent writing beyond the end of the ->ssid[] array.
Fixes: a2c60d42d97c ("staging: r8188eu: Add files for new driver - part 16")
Signed-off-by: Dan Carpenter
---
drivers/staging/rtl8188eu/os_dep/ioctl_linux.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
index bf22f130d3e1..58954b88a817 100644
--- a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
+++ b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
@@ -1133,9 +1133,11 @@ static int rtw_wx_set_scan(struct net_device *dev,
struct iw_request_info *a,
break;
}
sec_len = *(pos++); len -= 1;
- if (sec_len > 0 && sec_len <= len) {
+ if (sec_len > 0 &&
+ sec_len <= len &&
+ sec_len <= 32) {
ssid[ssid_index].ssid_length =
sec_len;
- memcpy(ssid[ssid_index].ssid,
pos, ssid[ssid_index].ssid_length);
+ memcpy(ssid[ssid_index].ssid,
pos, sec_len);
ssid_index++;
}
pos += sec_len;
--
2.30.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: rtl8188eu: fix potential memory corruption in rtw_check_beacon_data()
The "ie_len" is a value in the 1-255 range that comes from the user. We
have to cap it to ensure that it's not too large or it could lead to
memory corruption.
Fixes: 9a7fe54ddc3a ("staging: r8188eu: Add source files for new driver - part
1")
Signed-off-by: Dan Carpenter
---
drivers/staging/rtl8188eu/core/rtw_ap.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/staging/rtl8188eu/core/rtw_ap.c
b/drivers/staging/rtl8188eu/core/rtw_ap.c
index fa1e34a0d456..182bb944c9b3 100644
--- a/drivers/staging/rtl8188eu/core/rtw_ap.c
+++ b/drivers/staging/rtl8188eu/core/rtw_ap.c
@@ -791,6 +791,7 @@ int rtw_check_beacon_data(struct adapter *padapter, u8
*pbuf, int len)
p = rtw_get_ie(ie + _BEACON_IE_OFFSET_, WLAN_EID_SSID, _len,
pbss_network->ie_length - _BEACON_IE_OFFSET_);
if (p && ie_len > 0) {
+ ie_len = min_t(int, ie_len, sizeof(pbss_network->ssid.ssid));
memset(_network->ssid, 0, sizeof(struct ndis_802_11_ssid));
memcpy(pbss_network->ssid.ssid, p + 2, ie_len);
pbss_network->ssid.ssid_length = ie_len;
@@ -811,6 +812,7 @@ int rtw_check_beacon_data(struct adapter *padapter, u8
*pbuf, int len)
p = rtw_get_ie(ie + _BEACON_IE_OFFSET_, WLAN_EID_SUPP_RATES, _len,
pbss_network->ie_length - _BEACON_IE_OFFSET_);
if (p) {
+ ie_len = min_t(int, ie_len, NDIS_802_11_LENGTH_RATES_EX);
memcpy(supportRate, p + 2, ie_len);
supportRateNum = ie_len;
}
@@ -819,6 +821,8 @@ int rtw_check_beacon_data(struct adapter *padapter, u8
*pbuf, int len)
p = rtw_get_ie(ie + _BEACON_IE_OFFSET_, WLAN_EID_EXT_SUPP_RATES,
_len, pbss_network->ie_length - _BEACON_IE_OFFSET_);
if (p) {
+ ie_len = min_t(int, ie_len,
+ NDIS_802_11_LENGTH_RATES_EX - supportRateNum);
memcpy(supportRate + supportRateNum, p + 2, ie_len);
supportRateNum += ie_len;
}
@@ -934,6 +938,7 @@ int rtw_check_beacon_data(struct adapter *padapter, u8
*pbuf, int len)
pht_cap->mcs.rx_mask[0] = 0xff;
pht_cap->mcs.rx_mask[1] = 0x0;
+ ie_len = min_t(int, ie_len, sizeof(pmlmepriv->htpriv.ht_cap));
memcpy(>htpriv.ht_cap, p + 2, ie_len);
}
--
2.30.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan
Actually, I looked through a bunch of these and they're mostly false positives outside of staging. I guess there are a few ways the ->ssid can be changed. Via netlink, from the network or from the an ioctl. I still have a couple questions, but so far as I can see it's mostly the ioctl which has problems. I really want Smatch to be able to figure the netlink stuff... That should be doable. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: rtl8192u: fix ->ssid overflow in r8192_wx_set_scan()
We need to cap len at IW_ESSID_MAX_SIZE (32) to avoid memory corruption.
This can be controlled by the user via the ioctl.
Fixes: 5f53d8ca3d5d ("Staging: add rtl8192SU wireless usb driver")
Signed-off-by: Dan Carpenter
---
drivers/staging/rtl8192u/r8192U_wx.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/rtl8192u/r8192U_wx.c
b/drivers/staging/rtl8192u/r8192U_wx.c
index 175bb8b15389..5211b2005763 100644
--- a/drivers/staging/rtl8192u/r8192U_wx.c
+++ b/drivers/staging/rtl8192u/r8192U_wx.c
@@ -331,8 +331,10 @@ static int r8192_wx_set_scan(struct net_device *dev,
struct iw_request_info *a,
struct iw_scan_req *req = (struct iw_scan_req *)b;
if (req->essid_len) {
- ieee->current_network.ssid_len = req->essid_len;
- memcpy(ieee->current_network.ssid, req->essid,
req->essid_len);
+ int len = min_t(int, req->essid_len, IW_ESSID_MAX_SIZE);
+
+ ieee->current_network.ssid_len = len;
+ memcpy(ieee->current_network.ssid, req->essid, len);
}
}
--
2.30.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8192e: remove unnecessary break
On Tue, Mar 02, 2021 at 11:29:03PM +0800, Perrin Smith wrote: > From: Perrin Smith > > removed unnecessary break at end of while loop > > Signed-off-by: Perrin Smith > --- > drivers/staging/rtl8192e/rtllib_rx.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/staging/rtl8192e/rtllib_rx.c > b/drivers/staging/rtl8192e/rtllib_rx.c > index b8ab34250e6a..2de6330b7737 100644 > --- a/drivers/staging/rtl8192e/rtllib_rx.c > +++ b/drivers/staging/rtl8192e/rtllib_rx.c > @@ -460,8 +460,6 @@ static bool AddReorderEntry(struct rx_ts_record *pTS, > ((struct rx_reorder_entry *)list_entry(pList->next, > struct rx_reorder_entry, List))->SeqNum)) > return false; > - else > - break; No, this break is necessary. The patch introduces a bug. You need to be careful following checkpatch's advice because it's just a simple Perl script. > } regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: ks7010: prevent buffer overflow in ks_wlan_set_scan()
The user can specify a "req->essid_len" of up to 255 but if it's
over IW_ESSID_MAX_SIZE (32) that can lead to memory corruption.
Fixes: 13a9930d15b4 ("staging: ks7010: add driver from Nanonote
extra-repository")
Signed-off-by: Dan Carpenter
---
drivers/staging/ks7010/ks_wlan_net.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/ks7010/ks_wlan_net.c
b/drivers/staging/ks7010/ks_wlan_net.c
index dc09cc6e1c47..09e7b4cd0138 100644
--- a/drivers/staging/ks7010/ks_wlan_net.c
+++ b/drivers/staging/ks7010/ks_wlan_net.c
@@ -1120,6 +1120,7 @@ static int ks_wlan_set_scan(struct net_device *dev,
{
struct ks_wlan_private *priv = netdev_priv(dev);
struct iw_scan_req *req = NULL;
+ int len;
if (priv->sleep_mode == SLP_SLEEP)
return -EPERM;
@@ -1129,8 +1130,9 @@ static int ks_wlan_set_scan(struct net_device *dev,
if (wrqu->data.length == sizeof(struct iw_scan_req) &&
wrqu->data.flags & IW_SCAN_THIS_ESSID) {
req = (struct iw_scan_req *)extra;
- priv->scan_ssid_len = req->essid_len;
- memcpy(priv->scan_ssid, req->essid, priv->scan_ssid_len);
+ len = min_t(int, req->essid_len, IW_ESSID_MAX_SIZE);
+ priv->scan_ssid_len = len;
+ memcpy(priv->scan_ssid, req->essid, len);
} else {
priv->scan_ssid_len = 0;
}
--
2.30.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 3/3] staging: rtl8723bs: os_dep: Replace sprintf with scnprintf
On Mon, Mar 01, 2021 at 10:00:11PM +0700, Candy Febriyanto wrote: > The use of sprintf with format string here means that there is a risk > that the writes will go out of bounds, replace it with scnprintf. > > In one block of the translate_scan function sprintf is only called once > (it's not being used to concatenate strings) so there is no need to keep > the pointer "p", remove it. > > Signed-off-by: Candy Febriyanto > --- Looks good. TBH, v1 was also fine. I should have just acked it instead of commenting... Reviewed-by: Dan Carpenter regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/3] staging: rtl8723bs: os_dep: Replace sprintf with scnprintf
On Mon, Mar 01, 2021 at 08:13:54PM +0700, Candy Febriyanto wrote:
> @@ -5082,7 +5084,7 @@ static int rtw_ioctl_wext_private(struct net_device
> *dev, union iwreq_data *wrq_
> case IW_PRIV_TYPE_BYTE:
> /* Display args */
> for (j = 0; j < n; j++) {
> - sprintf(str, "%d ", extra[j]);
> + scnprintf(str, sizeof(str), "%d ", extra[j]);
> len = strlen(str);
You could save a little code and combine the two statements:
len = scnprintf(str, sizeof(str), "%d ",
extra[j]);
For bonus points, you could write a Coccinelle script to look for that
pattern of calling strlen() on a freshly sprintfed string.
> output_len = strlen(output);
> if ((output_len + len + 1) > 4096) {
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan
On Fri, Feb 26, 2021 at 05:05:26PM +0300, Dan Carpenter wrote:
> Here is a v2 of my check. I've changed it to mark all "->ssid" and
> everything in "(struct ieee80211_network)" as protected. I'm just
> playing around with it at this point to explore what works best. It's
> impossible to know until after some results come back.
>
[ Added linux-wireless to the CC list. Here was the original check I
wrote on Friday. https://lore.kernel.org/lkml/20210226140526.GG@kadam/ ]
This check worked out pretty well. It's probably 50% bugs? Unfiltered
results below. The trick of warning for "if (ststr(member, "->ssid")) "
and the memcpy length couldn't be verified turned out to be the best.
Protecting ieee80211_network didn't find anything.
Sometimes we're copying from an existing (presumably verified) config
to another config so the ->ssid_len is valid. An example of that is:
drivers/staging/rtl8192e/rtllib_softmac.c:1685 rtllib_softmac_new_net()
protected struct member '(struct rtllib_network)->ssid' overflow: rl='0-255'
drivers/staging/rtl8192e/rtllib_softmac.c
1674 /* Save the essid so that if it is hidden, it is
1675 * replaced with the essid provided by the user.
1676 */
1677 if (!ssidbroad) {
1678 memcpy(tmp_ssid,
ieee->current_network.ssid,
1679 ieee->current_network.ssid_len);
1680 tmp_ssid_len =
ieee->current_network.ssid_len;
We can assume the existing ssid_len is valid
1681 }
1682 memcpy(>current_network, net,
1683 sizeof(ieee->current_network));
1684 if (!ssidbroad) {
1685 memcpy(ieee->current_network.ssid,
tmp_ssid,
1686 tmp_ssid_len);
so this memcpy() won't overflow. It's easy enough for a human reviewer
to make this sort of assumption, but Smatch can't.
1687 ieee->current_network.ssid_len =
tmp_ssid_len;
1688 }
All the code outside of drivers/ seems correct. They're mostly similar
examples of copying the ssid from one valid existing config to another.
The other places are using nla attrs like this:
net/wireless/nl80211.c
14399 if (info->attrs[NL80211_ATTR_SSID]) {
14400 params.ssid.ssid_len =
nla_len(info->attrs[NL80211_ATTR_SSID]);
14401 if (params.ssid.ssid_len == 0)
14402 return -EINVAL;
14403 memcpy(params.ssid.ssid,
14404 nla_data(info->attrs[NL80211_ATTR_SSID]),
14405 params.ssid.ssid_len);
14406 }
Smatch doesn't parse nla attributes correctly. They're capped using
the nl80211_policy[] array:
net/wireless/nl80211.c
528 [NL80211_ATTR_SSID] = { .type = NLA_BINARY,
529 .len = IEEE80211_MAX_SSID_LEN },
But there are quite a few real bugs as well. If anyone wants to fix any
of these just claim a bug, and I won't send a patch for that warning.
:) Lee, I think you mentioned that you had found a related buffer
overflow fix? Did the check find it?
regards,
dan carpenter
drivers/staging/rtl8192u/r8192U_wx.c:335 r8192_wx_set_scan() protected struct
member '(struct ieee80211_network)->ssid' overflow: rl='1-255'
drivers/staging/rtl8192e/rtllib_softmac.c:1685 rtllib_softmac_new_net()
protected struct member '(struct rtllib_network)->ssid' overflow: rl='0-255'
drivers/staging/rtl8192e/rtl8192e/rtl_wx.c:412 _rtl92e_wx_set_scan() protected
struct member '(struct rtllib_network)->ssid' overflow: rl='1-255'
drivers/staging/rtl8188eu/core/rtw_ap.c:795 rtw_check_beacon_data() protected
struct member '(struct ndis_802_11_ssid)->ssid' overflow: rl='1-255'
drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:1138 rtw_wx_set_scan() protected
struct member '(struct ndis_802_11_ssid)->ssid' overflow: rl='1-127'
drivers/net/wireless/ath/wcn36xx/smd.c:1571 wcn36xx_smd_set_bss_params()
protected struct member '(struct wcn36xx_hal_mac_ssid)->ssid' overflow:
rl='0-255'
drivers/net/wireless/ath/ath11k/wmi.c:2148 ath11k_wmi_send_scan_start_cmd()
protected struct member '(struct wmi_ssid)->ssid' overflow: rl='0-255'
drivers/net/wireless/ath/wil6210/cfg80211.c:2100 wil_cfg80211_change_beacon()
protected struct member '(struct wil6210_vif)->ssid' overflow: rl='0-255'
drivers/net/wireless/ath/ath10k/wow.c:198 ath10k_wmi_pno_check() protected
struct member '(struct wmi_ssid)->ssid' overflow: rl
Re: [PATCH] staging: rtl8192u avoid flex array of flex array
On Sat, Feb 27, 2021 at 07:06:14PM -0600, Darryl T. Agostinelli wrote:
> Undo the flex array in struct ieee80211_info_element. It is used as the flex
> array type in other structs (creating a flex array of flex arrays) making
> sparse unhappy. This change maintains the intent of the code and satisfies
> sparse.
>
> Signed-off-by: Darryl T. Agostinelli
> ---
> drivers/staging/rtl8192u/ieee80211/ieee80211.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211.h
> b/drivers/staging/rtl8192u/ieee80211/ieee80211.h
> index 39f4ddd86796..43bb7aeb35e3 100644
> --- a/drivers/staging/rtl8192u/ieee80211/ieee80211.h
> +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211.h
> @@ -951,7 +951,7 @@ struct rtl_80211_hdr_4addrqos {
> struct ieee80211_info_element {
> u8 id;
> u8 len;
> - u8 data[];
> + u8 data[0];
Nah... Just ignore Sparse on this.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8723bs: Fixed indentation and coding style
On Sun, Feb 28, 2021 at 03:32:30AM +0530, chakravarthikulkarni wrote:
> @@ -795,14 +795,14 @@ struct RunInThread_param {
>
>
> /*
> -
> -Result:
> -0x00: success
> -0x01: sucess, and check Response.
> -0x02: cmd ignored due to duplicated sequcne number
> -0x03: cmd dropped due to invalid cmd code
> -0x04: reserved.
> -
> +*
> +*Result:
> +*0x00: success
> +*0x01: sucess, and check Response.
> +*0x02: cmd ignored due to duplicated sequcne number
> +*0x03: cmd dropped due to invalid cmd code
> +*0x04: reserved.
> +*
> */
This indenting style is wrong. There should be a spaces around the '*'
character:
/*
* Result:
* 0x00: success
* 0x01: sucess, and check Response.
* 0x02: cmd ignored due to duplicated sequcne number
* 0x03: cmd dropped due to invalid cmd code
* 0x04: reserved.
*/
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan
On Fri, Feb 26, 2021 at 02:51:57PM +, Lee Gibson wrote: > Function _rtl92e_wx_set_scan calls memcpy without checking the length. > A user could control that length and trigger a buffer overflow. > Fix by checking the length is within the maximum allowed size. > > Reviewed-by: Dan Carpenter > Signed-off-by: Lee Gibson > --- Thanks! regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan
Here is a v2 of my check. I've changed it to mark all "->ssid" and
everything in "(struct ieee80211_network)" as protected. I'm just
playing around with it at this point to explore what works best. It's
impossible to know until after some results come back.
regards,
dan carpenter
#include "smatch.h"
#include "smatch_slist.h"
#include "smatch_extra.h"
static int my_id;
static struct {
const char *type_name;
int len;
} member_list[] = {
{ "(struct ieee80211_network)->ssid", 32 },
{ "(struct rtllib_network)->ssid", 32 },
};
static void match_memset(const char *fn, struct expression *expr, void *_unused)
{
struct expression *dest, *size_arg;
struct range_list *rl;
char *member_name;
int dest_size = 0;
int i;
dest = get_argument_from_call_expr(expr->args, 0);
size_arg = get_argument_from_call_expr(expr->args, 2);
if (!dest || !size_arg)
return;
member_name = get_member_name(dest);
if (!member_name)
return;
for (i = 0; i < ARRAY_SIZE(member_list); i++) {
if (strcmp(member_name, member_list[i].type_name) == 0) {
dest_size = member_list[i].len;
goto check;
}
}
if (strstr(member_name, "->ssid"))
goto check;
if (strncmp(member_name, "(struct ieee80211_network)", 26) == 0)
goto check;
goto free;
check:
get_absolute_rl(size_arg, );
if (!dest_size)
dest_size = get_array_size_bytes(dest);
if (rl_max(rl).value <= dest_size)
goto free;
if (dest_size <= 0 && is_capped(size_arg))
goto free;
sm_msg("protected struct member '%s' overflow: rl='%s'", member_name,
show_rl(rl));
free:
free_string(member_name);
}
void check_protected_member(int id)
{
if (option_project != PROJ_KERNEL)
return;
my_id = id;
add_function_hook("memcpy", _memset, NULL);
add_function_hook("__memcpy", _memset, NULL);
}
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan
On Fri, Feb 26, 2021 at 01:27:25PM +, Lee Gibson wrote: > Function _rtl92e_wx_set_scan calls memcpy without checking the length. > A user could control that length and trigger a buffer overflow. > Fix by checking the length is within the maximum allowed size. > > Changes in v2: > Changed to use min_t as per useful suggestions This kind of information is supposed to go below the --- cut off line > > Signed-off-by: Lee Gibson > --- ^^^ here. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan
On Fri, Feb 26, 2021 at 11:48:29AM +, Lee Gibson wrote:
> Function _rtl92e_wx_set_scan calls memcpy without checking the length.
> A user could control that length and trigger a buffer overflow.
> Fix by checking the length is within the maximum allowed size.
>
> Signed-off-by: Lee Gibson
> ---
> drivers/staging/rtl8192e/rtl8192e/rtl_wx.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
> b/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
> index 16bcee13f64b..2acc4f314732 100644
> --- a/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
> +++ b/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
> @@ -406,6 +406,9 @@ static int _rtl92e_wx_set_scan(struct net_device *dev,
> struct iw_scan_req *req = (struct iw_scan_req *)b;
>
> if (req->essid_len) {
> + if (req->essid_len > IW_ESSID_MAX_SIZE)
> + req->essid_len = IW_ESSID_MAX_SIZE;
> +
> ieee->current_network.ssid_len = req->essid_len;
> memcpy(ieee->current_network.ssid, req->essid,
> req->essid_len);
> --
Well done. You can add a Reviewed-by: Dan Carpenter
to your final patch. How did you spot this bug? It's so ancient that
the Fixes tag would look messed up.
commit 94a799425eee8225a1e3fbe5f473d2ef04002577
Author: Larry Finger
Date: Tue Aug 23 19:00:42 2011 -0500
From: wlanfae
[PATCH 1/8] rtl8192e: Import new version of driver from realtek
Smatch isn't smart enough to track how this function is called. Smatch
tries to track the names of the pointers that a function can be. For
example, the pointer is stored in r8192_wx_handlers[] and it's returned
from get_handler(). Here is that list.
$ smdb.py function_ptr _rtl92e_wx_set_scan
_rtl92e_wx_set_scan = ['_rtl92e_wx_set_scan', 'r8192_wx_handlers[]', '(struct
iw_handler_def)->standard', 'r get_handler()', 'wireless_process_ioctl ptr
handler', 'standard param 4', 'private param 4']
But Smatch gets confused when we do:
net/wireless/wext-core.c
951 handler = get_handler(dev, cmd);
952 if (handler) {
953 /* Standard and private are not the same */
954 if (cmd < SIOCIWFIRSTPRIV)
955 return standard(dev, iwr, cmd, info, handler);
Passing the handler pointer to the standard() pointer...
956 else if (private)
957 return private(dev, iwr, cmd, info, handler);
958 }
I can hard code the correct function pointer by adding some insert
commands into the smatch_data/db/fixup_kernel.sh file.
insert into function_ptr values ("fixup_kernel.sh", "r get_handler()",
"ioctl_standard_call ptr param 4", 1);
insert into function_ptr values ("fixup_kernel.sh", "r get_handler()",
"ioctl_standard_iw_point param 3", 1);
And now it generates the warning
But I wonder if probably another idea is to just create a new warning
that any time we memcpy() to a (struct ieee80211_network)->ssid and the
length is not known to be less than IW_ESSID_MAX_SIZE then print a
warning.
It turns out this process was slightly more unwieldy than I expected.
Adding the types manually seems like it might be a lot of work. Someone
could probably go through the list of CVEs from last year and see which
types were overflowed. Anyway, I'll test out what I have and post my
results next week.
regards,
dan carpenter
#include "smatch.h"
#include "smatch_slist.h"
#include "smatch_extra.h"
static int my_id;
static struct {
const char *type_name;
int len;
} member_list[] = {
{ "(struct ieee80211_network)->ssid", 32 },
{ "(struct rtllib_network)->ssid", 32 },
};
static void match_memset(const char *fn, struct expression *expr, void *_unused)
{
struct expression *dest, *size_expr;
struct range_list *rl;
char *member_name;
int size;
int i;
dest = get_argument_from_call_expr(expr->args, 0);
size_expr = get_argument_from_call_expr(expr->args, 2);
if (!dest || !size_expr)
return;
member_name = get_member_name(dest);
if (!member_name)
return;
for (i = 0; i < ARRAY_SIZE(member_list); i++) {
if (strcmp(member_name, member_list[i].type_name) == 0)
break;
}
if (i == ARRAY_SIZE(member_list))
goto free;
if (member_list[i].len)
size = member_list[i].len;
else
size = get_array_size_bytes(dest);
get_absolute_rl(size_expr, );
if (rl_max(rl).value <= size)
Re: [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan
On Fri, Feb 26, 2021 at 01:06:46PM +0100, Greg KH wrote:
> On Fri, Feb 26, 2021 at 11:48:29AM +, Lee Gibson wrote:
> > Function _rtl92e_wx_set_scan calls memcpy without checking the length.
> > A user could control that length and trigger a buffer overflow.
> > Fix by checking the length is within the maximum allowed size.
> >
> > Signed-off-by: Lee Gibson
> > ---
> > drivers/staging/rtl8192e/rtl8192e/rtl_wx.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
> > b/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
> > index 16bcee13f64b..2acc4f314732 100644
> > --- a/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
> > +++ b/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
> > @@ -406,6 +406,9 @@ static int _rtl92e_wx_set_scan(struct net_device *dev,
> > struct iw_scan_req *req = (struct iw_scan_req *)b;
> >
> > if (req->essid_len) {
> > + if (req->essid_len > IW_ESSID_MAX_SIZE)
> > + req->essid_len = IW_ESSID_MAX_SIZE;
> > +
>
> How about using the "pattern" the other wireless drivers use of:
>
> int len = min((int)req->essid_len, IW_ESSID_MAX_SIZE);
I bet checkpatch complains that we should use min_t() instead (but I'm
too lazy to verify).
int len = min_t(int, req->essid_len, IW_ESSID_MAX_SIZE);
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 00/13] bss_ht struct cleanups
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 0/2] fix sparse warnings
On Wed, Feb 24, 2021 at 08:26:41PM +0530, karthek wrote: > On Sun, Feb 21, 2021 at 09:00:48PM +0530, karthik alapati wrote: > > the following patches fixes two byte-order issues > > and fixes these sparse warnings > > > > > > drivers/staging//wimax/i2400m/op-rfkill.c:89:25: warning: incorrect type in > > assignment (different base types) > > drivers/staging//wimax/i2400m/op-rfkill.c:89:25:expected restricted > > __le16 [usertype] length > > drivers/staging//wimax/i2400m/op-rfkill.c:89:25:got unsigned long > > . > > drivers/staging//wimax/i2400m/fw.c:514:27: warning: restricted __le32 > > degrades to integer > > > > > > karthik alapati (2): > > staging: wimax/i2400m: fix byte-order issue > > staging: wimax/i2400m: convert __le32 type to host byte-order > > > > drivers/staging/wimax/i2400m/fw.c| 2 +- > > drivers/staging/wimax/i2400m/op-rfkill.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > -- > > 2.30.1 > > > ping? The merge window is open so no one is merging these types of fixes now. Wait until -rc1 is out, and then give the maintainer two weeks to look at your patch and get back to you. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: rtl8712: unterminated string leads to read overflow
The memdup_user() function does not necessarily return a NUL terminated
string so this can lead to a read overflow. Switch from memdup_user()
to strndup_user() to fix this bug.
Fixes: c6dc001f2add ("staging: r8712u: Merging Realtek's latest (v2.6.6).
Various fixes.")
Signed-off-by: Dan Carpenter
---
drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
index 81de5a9e6b67..60dd798a6e51 100644
--- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
+++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
@@ -924,7 +924,7 @@ static int r871x_wx_set_priv(struct net_device *dev,
struct iw_point *dwrq = (struct iw_point *)awrq;
len = dwrq->length;
- ext = memdup_user(dwrq->pointer, len);
+ ext = strndup_user(dwrq->pointer, len);
if (IS_ERR(ext))
return PTR_ERR(ext);
--
2.30.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/13] staging: rtl8192e: remove blank line in bss_ht struct
On Sat, Feb 20, 2021 at 03:54:05PM +, William Durand wrote: > Fixes a checkpatch CHECK issue. > All these patches have the same vague commit message. It's okay if the commit message basically restates the commit one line summary. It should say something like: Fix a checkpatch warning about a blank line after an open curly brace. Rename FooBar to foo_bar to silence a checkpatch warning about CamelCase. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: wlan-ng/p80211 : check userspacebuf size for sanity
I have added the Driver Devel list to the CC list. Adding linux-kernel
is sort of useless. The correct people who are interested in this patch
are all on the Driver Devel list.
On Mon, Feb 22, 2021 at 07:12:22PM +0530, karthek wrote:
> On Mon, Feb 22, 2021 at 04:21:33PM +0300, Dan Carpenter wrote:
> > On Mon, Feb 22, 2021 at 06:16:24PM +0530, karthek wrote:
> > > currently p80211knetdev_do_ioctl() is testing user passed
> > > struct ifreq for sanity by checking for presence of a magic number,
> > > in addition to that also check size field, preventing buffer overflow
> > > before passing data to p80211req_dorequest() which casts it
> > > to *struct p80211msg
> > >
> > > Signed-off-by: karthek
> > > ---
> > > is this correct?
> > > is it necessary to check for size in addition to magicnum?
> > > did i even understand the problem correctly?
> > >
> > > drivers/staging/wlan-ng/p80211netdev.c | 5 -
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/wlan-ng/p80211netdev.c
> > > b/drivers/staging/wlan-ng/p80211netdev.c
> > > index 70570e8a5..c7b78d870 100644
> > > --- a/drivers/staging/wlan-ng/p80211netdev.c
> > > +++ b/drivers/staging/wlan-ng/p80211netdev.c
> > > @@ -568,7 +568,10 @@ static int p80211knetdev_do_ioctl(struct net_device
> > > *dev,
> > > result = -EINVAL;
> > > goto bail;
> > > }
> > > -
> > > + if (req->len < sizeof(struct p80211msg)) {
> > > + result = -EINVAL;
> > > + goto bail;
> > > + }
> >
> > Please don't send private emails. Always CC the list.
> sorry
> >
> > That's only a partial solution. You need to check in p80211req_handlemsg()
> > as well and probably other places.
> currently p80211req_handlemsg() is only referenced in p80211req_dorequest()
> can we check that there instead?
If I have to do all the work in finding the buffer overflows, then I
should write my own patch. :/
Anyway the p80211knetdev_do_ioctl() function calls p80211req_dorequest()
which calls p80211req_handlemsg(wlandev, msg); and
wlandev->mlmerequest(wlandev, msg);.
We have already discussed the p80211req_handlemsg() function. The
wlandev->mlmerequest() function is always just prism2sta_mlmerequest().
The prism2sta_mlmerequest() calls a bunch of functions and each of those
functions need to have a different limit check added to prevent memory
corruption.
Homework #1: Should we get rid of the wlandev->mlmerequest() pointer
and just call prism2sta_mlmerequest() directly?
Homework #2: Another solution is to just delete all these custom IOCTLs.
I don't know what they do so I don't know if they are necessary or not.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: wimax: fix sparse incorrect type issue
On Mon, Feb 22, 2021 at 11:31:48AM +0530, karthek wrote: > On Mon, Feb 22, 2021 at 11:10 AM Dan Carpenter > wrote: > > > > On Sat, Feb 20, 2021 at 05:04:00PM +0530, karthik alapati wrote: > > > fix sparse warning by casting to explicit user address-space > > > pointer type > > > > > > Signed-off-by: karthik alapati > > > --- > > > drivers/staging/wlan-ng/p80211netdev.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/staging/wlan-ng/p80211netdev.c > > > b/drivers/staging/wlan-ng/p80211netdev.c > > > index 6f9666dc0..70570e8a5 100644 > > > --- a/drivers/staging/wlan-ng/p80211netdev.c > > > +++ b/drivers/staging/wlan-ng/p80211netdev.c > > > @@ -569,7 +569,7 @@ static int p80211knetdev_do_ioctl(struct net_device > > > *dev, > > > goto bail; > > > } > > > > > > - msgbuf = memdup_user(req->data, req->len); > > > + msgbuf = memdup_user((void __user *)req->data, req->len); > > > > This doesn't fix anything it just silences the warning. Linus Torvalds > > worked very hard to create Sparse for the express purpose of printing > > the warning. People don't realize that warnings are very valuable > > because they show where the bugs are. > > > > Please look at this some more and figure out how to fix the warning. > > > > To be honest, I'm tempted to not accept any patch which doesn't also fix > > the buffer overflows when we pass: > > > > result = p80211req_dorequest(wlandev, msgbuf); > > > > How do we know that "msgbuf" is large enough? > > > > regards, > > dan carpenter > > > > Thanks dan but right after sending this patch i immediately replied to > it stating > to ignore this patch as i found this already applied in staging-testing branch > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing=3a8a144d2a754df45127c74e273fa166f690ba43 > It's still possible to fix this in the correct way and fix the buffer overflows. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: ks7010: Replace macros with do while loop.
On Mon, Feb 22, 2021 at 01:43:24AM +0530, chakravarthikulkarni wrote: > This commit fix errors found in checkpath.pl. > Error message is: > > It is a good idea to keep complex macros in do while loop. > Otherwise result may have side effect. > > Signed-off-by: chakravarthikulkarni This breaks the build. Also just ignore this checkpatch warning. The original defines are fine. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: wimax: Fix block comment style issue in stack.c
On Sun, Feb 21, 2021 at 10:07:59PM +0530, Amrit Khera wrote:
> diff --git a/drivers/staging/wimax/stack.c b/drivers/staging/wimax/stack.c
> index ace24a6dfd2d..345a022810ef 100644
> --- a/drivers/staging/wimax/stack.c
> +++ b/drivers/staging/wimax/stack.c
> @@ -57,17 +57,7 @@ MODULE_PARM_DESC(debug,
>
> /*
> * Authoritative source for the RE_STATE_CHANGE attribute policy
Delete the whole comment. This sentence doesn't make any sense by
itself once we have removed the rest.
> - *
> - * We don't really use it here, but /me likes to keep the definition
> - * close to where the data is generated.
> */
> -/*
> -static const struct nla_policy wimax_gnl_re_status_change[WIMAX_GNL_ATTR_MAX
> + 1] = {
> - [WIMAX_GNL_STCH_STATE_OLD] = { .type = NLA_U8 },
> - [WIMAX_GNL_STCH_STATE_NEW] = { .type = NLA_U8 },
> -};
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: wimax/i2400m: fix byte-order issue
On Sat, Feb 20, 2021 at 05:56:47PM +0530, karthik alapati wrote: > fix sparse byte-order warnings by converting host byte-order > types to le32 types > > Signed-off-by: karthik alapati This is a v2 patch... regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: wimax: fix sparse incorrect type issue
On Sat, Feb 20, 2021 at 05:04:00PM +0530, karthik alapati wrote: > fix sparse warning by casting to explicit user address-space > pointer type > > Signed-off-by: karthik alapati > --- > drivers/staging/wlan-ng/p80211netdev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/wlan-ng/p80211netdev.c > b/drivers/staging/wlan-ng/p80211netdev.c > index 6f9666dc0..70570e8a5 100644 > --- a/drivers/staging/wlan-ng/p80211netdev.c > +++ b/drivers/staging/wlan-ng/p80211netdev.c > @@ -569,7 +569,7 @@ static int p80211knetdev_do_ioctl(struct net_device *dev, > goto bail; > } > > - msgbuf = memdup_user(req->data, req->len); > + msgbuf = memdup_user((void __user *)req->data, req->len); This doesn't fix anything it just silences the warning. Linus Torvalds worked very hard to create Sparse for the express purpose of printing the warning. People don't realize that warnings are very valuable because they show where the bugs are. Please look at this some more and figure out how to fix the warning. To be honest, I'm tempted to not accept any patch which doesn't also fix the buffer overflows when we pass: result = p80211req_dorequest(wlandev, msgbuf); How do we know that "msgbuf" is large enough? regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] staging: rtl8723bs: fix code style comparison warning
On Fri, Feb 19, 2021 at 06:23:54PM +, Kurt Manucredo wrote: > > > checkpatch gives the following WARNING: > WARNING: Comparisons should place the constant on the right side of the test > this patch fixes the coding style warning. > > Signed-off-by: Kurt Manucredo > --- Looks okay to me. Thanks! Reviewed-by: Dan Carpenter regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: rtl8723bs: fix code style comparison warning
On Fri, Feb 19, 2021 at 02:50:53PM +, Kurt Manucredo wrote: > > > changes since previous version: > - change Subject line > - change commit message > - change commit message position to above signed-off-by > These comments need to go below the --- cut off line. > checkpatch gives the following WARNING: > WARNING: Comparisons should place the constant on the right side of the test > this patch fixes the coding style warning. > > Signed-off-by: Kurt Manucredo > --- ^^^ This one here. > drivers/staging/rtl8723bs/core/rtw_wlan_util.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: vt6656: fixed a CamelCase coding style issue.
You're not asking the right questions.
On Fri, Feb 19, 2021 at 03:28:35PM +0530, Selvakumar Elangovan wrote:
> This patch renames CamelCase macros uVar and uModulo into u_var and
> u_module in device.h
>
Is "u_var" a good name? What does the "u_" even mean?
> This issue was reported by checkpatch.pl
>
> Signed-off-by: Selvakumar Elangovan
> ---
> drivers/staging/vt6656/device.h | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/vt6656/device.h b/drivers/staging/vt6656/device.h
> index 947530fefe94..6615d356f74a 100644
> --- a/drivers/staging/vt6656/device.h
> +++ b/drivers/staging/vt6656/device.h
> @@ -385,11 +385,11 @@ struct vnt_private {
> struct ieee80211_low_level_stats low_stats;
> };
>
> -#define ADD_ONE_WITH_WRAP_AROUND(uVar, uModulo) {\
> - if ((uVar) >= ((uModulo) - 1)) \
> - (uVar) = 0; \
> +#define ADD_ONE_WITH_WRAP_AROUND(u_var, u_modulo) { \
> + if ((u_var) >= ((u_modulo) - 1))\
The \ is not aligned any more.
> + (u_var) = 0;\
> else\
> - (uVar)++; \
> + (u_var)++; \
> }
This macro is rubbish. How does the wrap around even make sense?
I hope that if you review the code a bit I think you will find that the
wrap around is impossible? Just fix the two callers and delete this
macro.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: android: Fix const keyword style issue in ashmem.c
On Fri, Feb 19, 2021 at 05:12:38PM +0530, Amrit Khera wrote: > This change fixes a checkpatch warning for "struct file_operations > should normally be const". > > Signed-off-by: Amrit Khera > --- > Changes in v2: > - Wrapped the commit description > - Build tested Heh. Nope. This breaks the build. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 2/2] staging: rtl8192u: fixed coding style of r8190_rtl8256.c
It's against the rules to send two patches with the same subject. Also both subjects are too vague. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: fwserial: Fix alignment of function parameters
On Fri, Feb 19, 2021 at 11:39:27AM +0100, Greg KH wrote: > On Fri, Feb 19, 2021 at 03:25:38PM +0530, 17UCS047_Prakash Dubey wrote: > > I was unable to align it right below the opening parenthesis, just by using > > tabs. And when I did that with spaces, the checkpatch yelled at me for > > using spaces. Any suggestions how to do this without using spaces? I am > > using vim, I will try to find a workaround meanwhile. > Your comment hasn't made it to the list yet. Sometimes there is a delay or maybe it was blocked for some reason (html email?). You are allowed to use spaces but you can't have 8 consecutive spaces and spaces are not allowed before a tab character. The way to align it is: ret = wait_event_interruptible_timeout(port->wait_tx, !test_bit(IN_TX, >flags), 10); [tab x6][space x7]!test_bit(IN_TX, >flags) regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: fwserial: match alignment with open parenthesis
On Fri, Feb 19, 2021 at 12:03:18PM +0300, Nikolay Kyx wrote: > This patch fixes the following checkpatch.pl check: > > CHECK: Alignment should match open parenthesis > > in file fwserial.c > > Additionally some style warnings remain valid here and could be fixed by > another patch. > Don't put comments like this in the git log, put them under the --- cut off line. > Signed-off-by: Nikolay Kyx > --- > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: cast to (unsigned int *)
On Fri, Feb 19, 2021 at 09:03:59AM +, David Laight wrote: > > It's kind of moot anyway because the patch is outdated. But the reason > > for the ___force is that the same `struct comedi_cmd` is used in both > > user and kernel contexts. In user contexts, the `chanlist` member > > points to user memory and in kernel contexts it points to kernel memory > > (copied from userspace). > > Can't you use a union of the user and kernel pointers? > (Possibly even anonymous?) > Although, ideally, keeping them in separate fields is better. > 8 bytes for a pointer isn't going make a fat lot of difference. > Creating a union is worse than adding casts. With the casts, at least you know that you're doing something dangerous. It's good that it looks scary because it is scary. Keeping them in separate fields is a good idea, but this is part of the user space API so it's not possible. The best we can do is adding some more comments so people know why we are doing the scary casts. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] use explicit host byte-order types in comparison
On Fri, Feb 19, 2021 at 05:51:59AM +0530, karthik alapati wrote: > convert le32 types to host byte-order types before > comparison > Already fixed. Please work against staging-next or linux-next. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8723bs: make if-statement checkpatch.pl conform
On Thu, Feb 18, 2021 at 07:50:27PM +, Kurt Manucredo wrote: > Signed-off-by: Kurt Manucredo > --- > > The preferred coding style is: > if (!StaAddr) > return; Above the Signed-off-by line. Also indenting is wrong. And it's hard to understand what you're saying. > > thank you mr. dan carpenter You're welcome. (These sorts of comments do go below the --- cut off line so that's fine.) regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: rtl8723bs: code-style fix
The subject is too vague. On Thu, Feb 18, 2021 at 04:33:10PM +, Kurt Manucredo wrote: > Signed-off-by: Kurt Manucredo > --- > > Checkpatch complains the constant needs to be on the right side of the > comparison. The preferred way is: > The commit message isn't complete and it has to go above the Signed-off-by line. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/2] staging: comedi: cast function output to assigned variable type
No problem. These days I have fibre to my house, but I still remember trying to clone the kernel when I could only buy 20MB of data at a time. :P regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: wlan-ng: Fix comments typos
On Wed, Feb 10, 2021 at 03:59:52PM +0100, Mairo Paul Rufus wrote: > Signed-off-by: Mairo Paul Rufus No commit message. It should say something like: Checkpatch complained about the accidental repeated words like "our our" so I fixed it. > diff --git a/drivers/staging/wlan-ng/prism2sta.c > b/drivers/staging/wlan-ng/prism2sta.c > index 8f25496188aa..e6dcb687e7a1 100644 > --- a/drivers/staging/wlan-ng/prism2sta.c > +++ b/drivers/staging/wlan-ng/prism2sta.c > @@ -461,7 +461,7 @@ u32 prism2sta_ifstate(struct wlandevice *wlandev, u32 > ifstate) > case WLAN_MSD_FWLOAD: > wlandev->msdstate = WLAN_MSD_RUNNING_PENDING; > /* Initialize the device+driver for full > - * operation. Note that this might me an FWLOAD to > + * operation. Note that this might me an FWLOAD This probably should be "might be". >* to RUNNING transition so we must not do a chip >* or board level reset. Note that on failure, > * the MSD state is set to HWPRESENT because we regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] fix comparisons - put constant on right side- eudyptula challenge 10
Fix the subject. Don't mention eudyptula.
[PATCH] Staging: rtl8723bs: put constant on right side of comparison
Add a commit message:
Checkpatch complains that the constant needs to be on the right hand
side of the comparion.
On Thu, Feb 18, 2021 at 03:54:40PM +, Kurt Manucredo wrote:
> Signed-off-by: Kurt Manucredo
> ---
> drivers/staging/rtl8723bs/core/rtw_wlan_util.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
> b/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
> index 975f2830e29e..089c6ec19373 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
> @@ -2146,7 +2146,7 @@ void rtw_get_sec_iv(struct adapter *padapter, u8
> *pcur_dot11txpn, u8 *StaAddr)
> struct security_priv *psecpriv = >securitypriv;
>
> memset(pcur_dot11txpn, 0, 8);
> - if (NULL == StaAddr)
> + if (StaAddr == NULL)
The prefered format for this is actually:
if (!StaAddr)
return;
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] fix comparisons - put constant on right side- eudyptula
On Thu, Feb 18, 2021 at 03:54:29PM +, Kurt Manucredo wrote: > > Dear linux kernel developers, > > for my eudyptula challenge it is required of me to fix a coding style > issue in the staging area in linux-next. I am aware that it is in > general frowned upon when submitting these sorts of patches. However, to > finish my 10th challenge I was tasked to do exactly that. So I ask you > kindly to pull this patch if possible. > > Thank you for your time, These patches are fine in staging. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
