Re: [PATCH v4] dt-bindings: net: dsa: ksz9477: add sample of switch bindings managed in i2c mode
From: Sergio Paracuellos Date: Wed, 19 Dec 2018 20:46:26 +0100 > Add device-tree binding example of the ksz9477 switch managed in i2c mode. > > Cc: devicet...@vger.kernel.org > Signed-off-by: Sergio Paracuellos I am assuming the devicetree folks will take this into their tree or similar. If I need to take this into my tree, please let me know. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: android: ion: Add chunk heaps instantiation
On 12/20/18 10:36 PM, Laura Abbott wrote:
> On 12/16/18 2:46 AM, Alexey Skidanov wrote:
>> Chunk heap instantiation should be supported for device tree platforms
>> and
>> non device tree platforms. For device tree platforms, it's a platform
>> specific code responsibility to retrieve the heap configuration data
>> and to call the appropriate API for heap creation. For non device tree
>> platforms, there is no defined way to create the heaps.
>>
>> This patch provides the way of chunk heaps creation using
>> "ion_chunk_heap=name:size@start" kernel boot parameter.
>>
>
> I've been thinking about this and I think it works but
> I'm still kind of torn about not having devicetree bindings.
> It doesn't _preclude_ devicetree bindings but I'd hate to
> merge this and then end up with something incompatible.
> I do want to support non-Android use cases too.
Sorry, what do you mean by non-Android cases?
>
> I'm also curious about the value of this heap with just PAGE_SIZE.
> The original purpose of the chunk heap was a carveout where
> you could easily get allocations large than PAGE_SIZE to
> reduce TLB pressure. Do you have another use case for the
> chunk heap?
It need to be fixed ... Obviously ... The minimum allocation size should
be parametrized
>
> Sumit, do you have any thoughts?
>
> Thanks,
> Laura
>
>> Link:
>> http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-November/128495.html
>>
>> Signed-off-by: Alexey Skidanov
>> ---
>> drivers/staging/android/ion/ion_chunk_heap.c | 96
>> +---
>> include/linux/ion.h | 18 ++
>> 2 files changed, 105 insertions(+), 9 deletions(-)
>> create mode 100644 include/linux/ion.h
>>
>> diff --git a/drivers/staging/android/ion/ion_chunk_heap.c
>> b/drivers/staging/android/ion/ion_chunk_heap.c
>> index 102c093..1a8e3ad 100644
>> --- a/drivers/staging/android/ion/ion_chunk_heap.c
>> +++ b/drivers/staging/android/ion/ion_chunk_heap.c
>> @@ -21,8 +21,12 @@
>> #include
>> #include
>> #include
>> +#include
>> #include "ion.h"
>> +static struct ion_chunk_heap_cfg
>> chunk_heap_cfg[MAX_NUM_OF_CHUNK_HEAPS];
>> +static unsigned int num_of_req_chunk_heaps;
>> +
>> struct ion_chunk_heap {
>> struct ion_heap heap;
>> struct gen_pool *pool;
>> @@ -117,15 +121,15 @@ static struct ion_heap_ops chunk_heap_ops = {
>> .unmap_kernel = ion_heap_unmap_kernel,
>> };
>> -struct ion_heap *ion_chunk_heap_create(struct ion_platform_heap
>> *heap_data)
>> +static struct ion_heap *ion_chunk_heap_create(struct
>> ion_chunk_heap_cfg *heap_cfg)
>> {
>> struct ion_chunk_heap *chunk_heap;
>> int ret;
>> struct page *page;
>> size_t size;
>> - page = pfn_to_page(PFN_DOWN(heap_data->base));
>> - size = heap_data->size;
>> + page = pfn_to_page(PFN_DOWN(heap_cfg->base));
>> + size = heap_cfg->size;
>> ret = ion_heap_pages_zero(page, size,
>> pgprot_writecombine(PAGE_KERNEL));
>> if (ret)
>> @@ -135,23 +139,27 @@ struct ion_heap *ion_chunk_heap_create(struct
>> ion_platform_heap *heap_data)
>> if (!chunk_heap)
>> return ERR_PTR(-ENOMEM);
>> - chunk_heap->chunk_size = (unsigned long)heap_data->priv;
>> + chunk_heap->chunk_size = heap_cfg->chunk_size;
>> chunk_heap->pool =
>> gen_pool_create(get_order(chunk_heap->chunk_size) +
>> PAGE_SHIFT, -1);
>> if (!chunk_heap->pool) {
>> ret = -ENOMEM;
>> goto error_gen_pool_create;
>> }
>> - chunk_heap->base = heap_data->base;
>> - chunk_heap->size = heap_data->size;
>> + chunk_heap->base = heap_cfg->base;
>> + chunk_heap->size = heap_cfg->size;
>> + chunk_heap->heap.name = heap_cfg->heap_name;
>> chunk_heap->allocated = 0;
>> - gen_pool_add(chunk_heap->pool, chunk_heap->base,
>> heap_data->size, -1);
>> + gen_pool_add(chunk_heap->pool, chunk_heap->base, heap_cfg->size,
>> -1);
>> chunk_heap->heap.ops = &chunk_heap_ops;
>> chunk_heap->heap.type = ION_HEAP_TYPE_CHUNK;
>> chunk_heap->heap.flags = ION_HEAP_FLAG_DEFER_FREE;
>> - pr_debug("%s: base %pa size %zu\n", __func__,
>> - &chunk_heap->base, heap_data->size);
>> +
>> + pr_info("%s: name %s base %pa size %zu\n", __func__,
>> + heap_cfg->heap_name,
>> + &heap_cfg->base,
>> + heap_cfg->size);
>> return &chunk_heap->heap;
>> @@ -160,3 +168,73 @@ struct ion_heap *ion_chunk_heap_create(struct
>> ion_platform_heap *heap_data)
>> return ERR_PTR(ret);
>> }
>> +static int __init setup_heap(char *param)
>> +{
>> + char *at_sign, *coma, *colon;
>> + size_t size_to_copy;
>> + struct ion_chunk_heap_cfg *cfg;
>> +
>> + do {
>> + cfg = &chunk_heap_cfg[num_of_req_chunk_heaps];
>> +
>> + /* heap name */
>> + colon = strchr(param, ':');
>> + if (!colon)
>> + return -EINVAL;
>> +
>> + size_to_copy = min_t(size_t, MAX_CHUNK_
Re: [RESEND] staging: rtlwifi: convert to DEFINE_SHOW_ATTRIBUTE
Hi Yangtao, Thank you for the patch! Yet something to improve: [auto build test ERROR on staging/staging-testing] [also build test ERROR on v4.20-rc7 next-20181220] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Yangtao-Li/staging-rtlwifi-convert-to-DEFINE_SHOW_ATTRIBUTE/20181215-194101 config: x86_64-allmodconfig (attached as .config) compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): drivers/staging//rtlwifi/debug.c: In function 'rtl_debug_add_one': >> drivers/staging//rtlwifi/debug.c:519:46: error: 'commonfops' undeclared >> (first use in this function); did you mean 'common_fops'? RTL_DEBUGFS_ADD_CORE(name, S_IFREG | 0444, common) ^ drivers/staging//rtlwifi/debug.c:515:10: note: in definition of macro 'RTL_DEBUGFS_ADD_CORE' &fopname ##fops); \ ^~~ drivers/staging//rtlwifi/debug.c:545:2: note: in expansion of macro 'RTL_DEBUGFS_ADD' RTL_DEBUGFS_ADD(mac_0); ^~~ drivers/staging//rtlwifi/debug.c:519:46: note: each undeclared identifier is reported only once for each function it appears in RTL_DEBUGFS_ADD_CORE(name, S_IFREG | 0444, common) ^ drivers/staging//rtlwifi/debug.c:515:10: note: in definition of macro 'RTL_DEBUGFS_ADD_CORE' &fopname ##fops); \ ^~~ drivers/staging//rtlwifi/debug.c:545:2: note: in expansion of macro 'RTL_DEBUGFS_ADD' RTL_DEBUGFS_ADD(mac_0); ^~~ >> drivers/staging//rtlwifi/debug.c:521:46: error: 'common_writefops' >> undeclared (first use in this function); did you mean 'common_write_fops'? RTL_DEBUGFS_ADD_CORE(name, S_IFREG | 0222, common_write) ^ drivers/staging//rtlwifi/debug.c:515:10: note: in definition of macro 'RTL_DEBUGFS_ADD_CORE' &fopname ##fops); \ ^~~ drivers/staging//rtlwifi/debug.c:586:2: note: in expansion of macro 'RTL_DEBUGFS_ADD_W' RTL_DEBUGFS_ADD_W(write_reg); ^ >> drivers/staging//rtlwifi/debug.c:523:46: error: 'common_rwfops' undeclared >> (first use in this function); did you mean 'common_rw_fops'? RTL_DEBUGFS_ADD_CORE(name, S_IFREG | 0666, common_rw) ^ drivers/staging//rtlwifi/debug.c:515:10: note: in definition of macro 'RTL_DEBUGFS_ADD_CORE' &fopname ##fops); \ ^~~ drivers/staging//rtlwifi/debug.c:590:2: note: in expansion of macro 'RTL_DEBUGFS_ADD_RW' RTL_DEBUGFS_ADD_RW(phydm_cmd); ^~ -- >> drivers/staging/rtlwifi/debug.c:545:9: error: undefined identifier >> 'commonfops' drivers/staging/rtlwifi/debug.c:546:9: error: undefined identifier 'commonfops' drivers/staging/rtlwifi/debug.c:547:9: error: undefined identifier 'commonfops' drivers/staging/rtlwifi/debug.c:548:9: error: undefined identifier 'commonfops' drivers/staging/rtlwifi/debug.c:549:9: error: undefined identifier 'commonfops' drivers/staging/rtlwifi/debug.c:550:9: error: undefined identifier 'commonfops' drivers/staging/rtlwifi/debug.c:551:9: error: undefined identifier 'commonfops' drivers/staging/rtlwifi/debug.c:552:9: error: undefined identifier 'commonfops' drivers/staging/rtlwifi/debug.c:553:9: error: undefined identifier 'commonfops' drivers/staging/rtlwifi/debug.c:554:9: error: undefined identifier 'commonfops' drivers/staging/rtlwifi/debug.c:555:9: error: undefined identifier 'commonfops' drivers/staging/rtlwifi/debug.c:556:9: error: undefined identifier 'commonfops' drivers/staging/rtlwifi/debug.c:557:9: error: undefined identifier 'commonfops' drivers/staging/rtlwifi/debug.c:558:9: error: undefined identifier 'commonfops' drivers/staging/rtlwifi/debug.c:559:9: error: undefined identifier 'commonfops' drivers/staging/rtlwifi/debug.c:560:9: error: undefined identifier 'commonfops' drivers/staging/rtlwifi/debug.c:561:9: error: undefined identifier 'commonfops' drivers/staging/rtlwifi/debug.c:562:9: error: undefined identifier 'commonfops' drivers/staging/rtlwifi/debug.c:563:9: error: undefined identifier 'commonfops' drivers/staging/rtlwifi/debug.c:564:9: error: undefined identifier 'co
Re: [PATCH] staging: android: ion: add buffer flag update ioctl
On 12/19/18 5:39 PM, Zengtao (B) wrote:
Hi laura:
-Original Message-
From: Laura Abbott [mailto:labb...@redhat.com]
Sent: Thursday, December 20, 2018 2:10 AM
To: Zengtao (B) ; sumit.sem...@linaro.org
Cc: Greg Kroah-Hartman ; Arve Hjønnevåg
; Todd Kjos ; Martijn Coenen
; Joel Fernandes ;
de...@driverdev.osuosl.org; dri-de...@lists.freedesktop.org;
linaro-mm-...@lists.linaro.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH] staging: android: ion: add buffer flag update ioctl
On 12/19/18 9:19 AM, Zeng Tao wrote:
In some usecases, the buffer cached attribute is not determined at
allocation time, it's determined just before the real cpu mapping.
And from the memory view of point, a buffer should not have the
cached
attribute util is really mapped by the cpu. So in this patch, we
introduced the new ioctl command to target the requirement.
This is racy and error prone. Can you explain more what problem you are
trying to solve?
My use case is like this:
1. There are two process A and B, A takes case of ion buffer allocation, and
pass the buffer fd to B, then B maps and uses it.
2. Process B need to map the buffer with different cached attribute for
different use case, for example, if the buffer is used for pure software
algorithm,
then we need to map it as cached, otherwise non-cached, and B needs to deal
with both cases.
And unfortunately the mmap syscall takes no cached flags and we can't decide
the cache attribute when we are doing the mmap, so I introduce new the ioctl
even though I think the solution is not as good.
Thanks for the explanation, this was about the use case I expected.
I'm pretty sure I had this exact problem once upon a time and we
didn't come up with a solution. I'd still like to get rid of
uncached buffers in general and just use cached buffers
(see
http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-November/128842.html)
What's your usecase for uncached buffers?
Signed-off-by: Zeng Tao
---
drivers/staging/android/ion/ion-ioctl.c | 4
drivers/staging/android/ion/ion.c | 17 +
drivers/staging/android/ion/ion.h | 1 +
drivers/staging/android/uapi/ion.h | 22
++
4 files changed, 44 insertions(+)
diff --git a/drivers/staging/android/ion/ion-ioctl.c
b/drivers/staging/android/ion/ion-ioctl.c
index a8d3cc4..60bb702 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -12,6 +12,7 @@
union ion_ioctl_arg {
struct ion_allocation_data allocation;
+ struct ion_buffer_flag_data update;
struct ion_heap_query query;
};
@@ -83,6 +84,9 @@ long ion_ioctl(struct file *filp, unsigned int cmd,
unsigned long arg)
break;
}
+ case ION_IOC_BUFFER_UPDATE:
+ ret = ion_buffer_update(data.update.fd, data.update.flags);
+ break;
case ION_IOC_HEAP_QUERY:
ret = ion_query_heaps(&data.query);
break;
diff --git a/drivers/staging/android/ion/ion.c
b/drivers/staging/android/ion/ion.c
index 9907332..f1404dc 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -436,6 +436,23 @@ int ion_alloc(size_t len, unsigned int
heap_id_mask, unsigned int flags)
return fd;
}
+int ion_buffer_update(unsigned int fd, unsigned int flags) {
+ struct dma_buf *dmabuf;
+ struct ion_buffer *buffer;
+
+ dmabuf = dma_buf_get(fd);
+
+ if (!dmabuf)
+ return -EINVAL;
+
+ buffer = dmabuf->priv;
+ buffer->flags = flags;
+ dma_buf_put(dmabuf);
+
+ return 0;
+}
+
int ion_query_heaps(struct ion_heap_query *query)
{
struct ion_device *dev = internal_dev; diff --git
a/drivers/staging/android/ion/ion.h
b/drivers/staging/android/ion/ion.h
index c006fc1..99bf9ab 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -199,6 +199,7 @@ int ion_heap_pages_zero(struct page *page,
size_t size, pgprot_t pgprot);
int ion_alloc(size_t len,
unsigned int heap_id_mask,
unsigned int flags);
+int ion_buffer_update(unsigned int fd, unsigned int flags);
/**
* ion_heap_init_shrinker
diff --git a/drivers/staging/android/uapi/ion.h
b/drivers/staging/android/uapi/ion.h
index 5d70098..99753fc 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -74,6 +74,20 @@ struct ion_allocation_data {
__u32 unused;
};
+/**
+ * struct ion_buffer_flag_data - metadata passed from userspace for
+update
+ * buffer flags
+ * @fd:file descriptor of the buffer
+ * @flags: flags passed to the buffer
+ *
+ * Provided by userspace as an argument to the ioctl */
+
+struct ion_buffer_flag_data {
+ __u32 fd;
+ __u32 flags;
+}
+
#define MAX_HEAP_NAME32
/**
@@ -116,6 +130,14 @@ struc
Re: [PATCH] staging: android: ion: Add chunk heaps instantiation
On 12/16/18 2:46 AM, Alexey Skidanov wrote:
Chunk heap instantiation should be supported for device tree platforms and
non device tree platforms. For device tree platforms, it's a platform
specific code responsibility to retrieve the heap configuration data
and to call the appropriate API for heap creation. For non device tree
platforms, there is no defined way to create the heaps.
This patch provides the way of chunk heaps creation using
"ion_chunk_heap=name:size@start" kernel boot parameter.
I've been thinking about this and I think it works but
I'm still kind of torn about not having devicetree bindings.
It doesn't _preclude_ devicetree bindings but I'd hate to
merge this and then end up with something incompatible.
I do want to support non-Android use cases too.
I'm also curious about the value of this heap with just PAGE_SIZE.
The original purpose of the chunk heap was a carveout where
you could easily get allocations large than PAGE_SIZE to
reduce TLB pressure. Do you have another use case for the
chunk heap?
Sumit, do you have any thoughts?
Thanks,
Laura
Link:
http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-November/128495.html
Signed-off-by: Alexey Skidanov
---
drivers/staging/android/ion/ion_chunk_heap.c | 96 +---
include/linux/ion.h | 18 ++
2 files changed, 105 insertions(+), 9 deletions(-)
create mode 100644 include/linux/ion.h
diff --git a/drivers/staging/android/ion/ion_chunk_heap.c
b/drivers/staging/android/ion/ion_chunk_heap.c
index 102c093..1a8e3ad 100644
--- a/drivers/staging/android/ion/ion_chunk_heap.c
+++ b/drivers/staging/android/ion/ion_chunk_heap.c
@@ -21,8 +21,12 @@
#include
#include
#include
+#include
#include "ion.h"
+static struct ion_chunk_heap_cfg chunk_heap_cfg[MAX_NUM_OF_CHUNK_HEAPS];
+static unsigned int num_of_req_chunk_heaps;
+
struct ion_chunk_heap {
struct ion_heap heap;
struct gen_pool *pool;
@@ -117,15 +121,15 @@ static struct ion_heap_ops chunk_heap_ops = {
.unmap_kernel = ion_heap_unmap_kernel,
};
-struct ion_heap *ion_chunk_heap_create(struct ion_platform_heap *heap_data)
+static struct ion_heap *ion_chunk_heap_create(struct ion_chunk_heap_cfg
*heap_cfg)
{
struct ion_chunk_heap *chunk_heap;
int ret;
struct page *page;
size_t size;
- page = pfn_to_page(PFN_DOWN(heap_data->base));
- size = heap_data->size;
+ page = pfn_to_page(PFN_DOWN(heap_cfg->base));
+ size = heap_cfg->size;
ret = ion_heap_pages_zero(page, size, pgprot_writecombine(PAGE_KERNEL));
if (ret)
@@ -135,23 +139,27 @@ struct ion_heap *ion_chunk_heap_create(struct
ion_platform_heap *heap_data)
if (!chunk_heap)
return ERR_PTR(-ENOMEM);
- chunk_heap->chunk_size = (unsigned long)heap_data->priv;
+ chunk_heap->chunk_size = heap_cfg->chunk_size;
chunk_heap->pool = gen_pool_create(get_order(chunk_heap->chunk_size) +
PAGE_SHIFT, -1);
if (!chunk_heap->pool) {
ret = -ENOMEM;
goto error_gen_pool_create;
}
- chunk_heap->base = heap_data->base;
- chunk_heap->size = heap_data->size;
+ chunk_heap->base = heap_cfg->base;
+ chunk_heap->size = heap_cfg->size;
+ chunk_heap->heap.name = heap_cfg->heap_name;
chunk_heap->allocated = 0;
- gen_pool_add(chunk_heap->pool, chunk_heap->base, heap_data->size, -1);
+ gen_pool_add(chunk_heap->pool, chunk_heap->base, heap_cfg->size, -1);
chunk_heap->heap.ops = &chunk_heap_ops;
chunk_heap->heap.type = ION_HEAP_TYPE_CHUNK;
chunk_heap->heap.flags = ION_HEAP_FLAG_DEFER_FREE;
- pr_debug("%s: base %pa size %zu\n", __func__,
-&chunk_heap->base, heap_data->size);
+
+ pr_info("%s: name %s base %pa size %zu\n", __func__,
+ heap_cfg->heap_name,
+ &heap_cfg->base,
+ heap_cfg->size);
return &chunk_heap->heap;
@@ -160,3 +168,73 @@ struct ion_heap *ion_chunk_heap_create(struct ion_platform_heap *heap_data)
return ERR_PTR(ret);
}
+static int __init setup_heap(char *param)
+{
+ char *at_sign, *coma, *colon;
+ size_t size_to_copy;
+ struct ion_chunk_heap_cfg *cfg;
+
+ do {
+ cfg = &chunk_heap_cfg[num_of_req_chunk_heaps];
+
+ /* heap name */
+ colon = strchr(param, ':');
+ if (!colon)
+ return -EINVAL;
+
+ size_to_copy = min_t(size_t, MAX_CHUNK_HEAP_NAME_SIZE - 1,
+(colon - param));
+ strncpy(cfg->heap_name, param, size_to_copy);
+ cfg->heap_name[size_to_copy] = '\0';
+
+ /* heap size */
+ cfg->size = memparse((colon + 1), &at_sign);
+ if ((colon + 1) == at_sign)
+
Re: [PATCH] rts5208: fix a missing check of ms read
LGTM. Reviewed-by: Dan Carpenter regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] rts5208: fix a missing check of ms read
When ms_read_extra_data fails, the data read in "extra" buffer is just incorrect and thus should be used. However, "extra" is used in multiple places no matter ms_read_extra_data() fails or not. The fix checks the status of ms_read_extra_data() and returns with an error code upon its failure. Signed-off-by: Kangjie Lu --- drivers/staging/rts5208/ms.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/staging/rts5208/ms.c b/drivers/staging/rts5208/ms.c index f53adf15c685..03b359ec69e6 100644 --- a/drivers/staging/rts5208/ms.c +++ b/drivers/staging/rts5208/ms.c @@ -1676,7 +1676,9 @@ static int ms_copy_page(struct rtsx_chip *chip, u16 old_blk, u16 new_blk, return STATUS_FAIL; } - ms_read_extra_data(chip, old_blk, i, extra, MS_EXTRA_SIZE); + retval = ms_read_extra_data(chip, old_blk, i, extra, MS_EXTRA_SIZE); + if (retval != STATUS_SUCCESS) + return STATUS_FAIL; retval = ms_set_rw_reg_addr(chip, OverwriteFlag, MS_EXTRA_SIZE, SystemParm, 6); -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 10/11] staging: iio: adc: ad7606: Move out of staging
Hi Stefan, I love your patch! Yet something to improve: [auto build test ERROR on iio/togreg] [cannot apply to v4.20-rc7] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Stefan-Popa/staging-iio-adc-ad7606-Simplify-the-Kconfing-menu/20181216-132154 base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg config: um-allmodconfig (attached as .config) compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=um All errors (new ones prefixed by >>): >> ERROR: "devm_ioremap_resource" [drivers/iio/adc/ad7606_par.ko] undefined! --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] rts5208: add a missing check for the status of command sending
Hi Kangjie,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on staging/staging-testing]
[also build test WARNING on v4.20-rc7 next-20181220]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Kangjie-Lu/rts5208-add-a-missing-check-for-the-status-of-command-sending/20181221-004836
config: i386-randconfig-x005-201850 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
drivers/staging//rts5208/ms.c: In function 'mspro_rw_multi_sector':
>> drivers/staging//rts5208/ms.c:2722:3: warning: this 'if' clause does not
>> guard... [-Wmisleading-indentation]
if (val & MS_INT_BREQ)
^~
drivers/staging//rts5208/ms.c:2724:4: note: ...this statement, but the
latter is misleadingly indented as if it were guarded by the 'if'
if (retval != STATUS_SUCCESS)
^~
vim +/if +2722 drivers/staging//rts5208/ms.c
fa590c222f Micky Ching 2013-11-12 2605
fa590c222f Micky Ching 2013-11-12 2606 static int
mspro_rw_multi_sector(struct scsi_cmnd *srb,
fa590c222f Micky Ching 2013-11-12 2607 struct
rtsx_chip *chip, u32 start_sector,
fa590c222f Micky Ching 2013-11-12 2608 u16
sector_cnt)
fa590c222f Micky Ching 2013-11-12 2609 {
d1af6476e6 Dilek Uzulmez 2016-02-23 2610 struct ms_info *ms_card =
&chip->ms_card;
11201769d1 Quentin Lambert 2015-03-04 2611 bool mode_2k = false;
11201769d1 Quentin Lambert 2015-03-04 2612 int retval;
fa590c222f Micky Ching 2013-11-12 2613 u16 count;
fa590c222f Micky Ching 2013-11-12 2614 u8 val, trans_mode, rw_tpc,
rw_cmd;
fa590c222f Micky Ching 2013-11-12 2615
fa590c222f Micky Ching 2013-11-12 2616 ms_set_err_code(chip,
MS_NO_ERROR);
fa590c222f Micky Ching 2013-11-12 2617
fa590c222f Micky Ching 2013-11-12 2618 ms_card->cleanup_counter = 0;
fa590c222f Micky Ching 2013-11-12 2619
fa590c222f Micky Ching 2013-11-12 2620 if (CHK_MSHG(ms_card)) {
fa590c222f Micky Ching 2013-11-12 2621 if ((start_sector % 4)
|| (sector_cnt % 4)) {
fa590c222f Micky Ching 2013-11-12 2622 if
(srb->sc_data_direction == DMA_FROM_DEVICE) {
fa590c222f Micky Ching 2013-11-12 2623 rw_tpc
= PRO_READ_LONG_DATA;
fa590c222f Micky Ching 2013-11-12 2624 rw_cmd
= PRO_READ_DATA;
fa590c222f Micky Ching 2013-11-12 2625 } else {
fa590c222f Micky Ching 2013-11-12 2626 rw_tpc
= PRO_WRITE_LONG_DATA;
fa590c222f Micky Ching 2013-11-12 2627 rw_cmd
= PRO_WRITE_DATA;
fa590c222f Micky Ching 2013-11-12 2628 }
fa590c222f Micky Ching 2013-11-12 2629 } else {
fa590c222f Micky Ching 2013-11-12 2630 if
(srb->sc_data_direction == DMA_FROM_DEVICE) {
fa590c222f Micky Ching 2013-11-12 2631 rw_tpc
= PRO_READ_QUAD_DATA;
fa590c222f Micky Ching 2013-11-12 2632 rw_cmd
= PRO_READ_2K_DATA;
fa590c222f Micky Ching 2013-11-12 2633 } else {
fa590c222f Micky Ching 2013-11-12 2634 rw_tpc
= PRO_WRITE_QUAD_DATA;
fa590c222f Micky Ching 2013-11-12 2635 rw_cmd
= PRO_WRITE_2K_DATA;
fa590c222f Micky Ching 2013-11-12 2636 }
11201769d1 Quentin Lambert 2015-03-04 2637 mode_2k = true;
fa590c222f Micky Ching 2013-11-12 2638 }
fa590c222f Micky Ching 2013-11-12 2639 } else {
fa590c222f Micky Ching 2013-11-12 2640 if
(srb->sc_data_direction == DMA_FROM_DEVICE) {
fa590c222f Micky Ching 2013-11-12 2641 rw_tpc =
PRO_READ_LONG_DATA;
fa590c222f Micky Ching 2013-11-12 2642 rw_cmd =
PRO_READ_DATA;
fa590c222f Micky Ching 2013-11-12 2643 } else {
fa590c222f Micky Ching 2013-11-12 2644 rw_tpc =
PRO_WRITE_LONG_DATA;
fa590c222f Micky Ching 2013-11-12 2645 rw_cmd =
PRO_WRITE_DATA;
fa590c222f Micky Ching 2013-11-12 2646 }
fa590c222f Micky Ching 2013-11-12 2647 }
fa590c222f Micky Ching 2013-11-12 2648
fa590c222f Micky Ching 2013-11-12 2649 retval = ms_switch_clock(chip);
9f902b495b Aymen Qader 2018-09-20 2650 if (retval != STATUS_SUCCESS)
031366ea65 Joe Perches 2015-03-25 2651 return STATUS_FAIL;
fa590c222f Micky Ching 2013-11-12 2652
fa590c222f Micky Ching
[PATCH] staging: fsl-dpaa2: fix SPDX identifiers in header files
Use the correct comment style for SPDX identifiers in header files. Reported by checkpatch. Signed-off-by: Michael Straube --- drivers/staging/fsl-dpaa2/ethsw/dpsw-cmd.h | 2 +- drivers/staging/fsl-dpaa2/ethsw/dpsw.h | 2 +- drivers/staging/fsl-dpaa2/ethsw/ethsw.h| 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/staging/fsl-dpaa2/ethsw/dpsw-cmd.h b/drivers/staging/fsl-dpaa2/ethsw/dpsw-cmd.h index da744f2b0ee6..14b974defa3a 100644 --- a/drivers/staging/fsl-dpaa2/ethsw/dpsw-cmd.h +++ b/drivers/staging/fsl-dpaa2/ethsw/dpsw-cmd.h @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: GPL-2.0 +/* SPDX-License-Identifier: GPL-2.0 */ /* * Copyright 2014-2016 Freescale Semiconductor Inc. * Copyright 2017-2018 NXP diff --git a/drivers/staging/fsl-dpaa2/ethsw/dpsw.h b/drivers/staging/fsl-dpaa2/ethsw/dpsw.h index db43fa3782b8..25635259ce44 100644 --- a/drivers/staging/fsl-dpaa2/ethsw/dpsw.h +++ b/drivers/staging/fsl-dpaa2/ethsw/dpsw.h @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: GPL-2.0 +/* SPDX-License-Identifier: GPL-2.0 */ /* * Copyright 2014-2016 Freescale Semiconductor Inc. * Copyright 2017-2018 NXP diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw.h b/drivers/staging/fsl-dpaa2/ethsw/ethsw.h index 069c99bfba74..c48783680a05 100644 --- a/drivers/staging/fsl-dpaa2/ethsw/ethsw.h +++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw.h @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: GPL-2.0 +/* SPDX-License-Identifier: GPL-2.0 */ /* * DPAA2 Ethernet Switch declarations * -- 2.20.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] staging: mt7621-mmc: Correct spelling mistakes in comments
On Thu, Dec 20, 2018 at 05:57:37PM +0100, Jona Crasselt wrote: > Changed "avaiable" to "available" and "interupt" to "interrupt". > > Signed-off-by: Jona Crasselt > Signed-off-by: Felix Windsheimer > --- > drivers/staging/mt7621-mmc/sd.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) You forgot to put what changed from v1->v2 and from v2->v3 below the --- line. I'll go queue this up now as my tree is about to close for this merge window, but next time be more careful. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3] staging: mt7621-mmc: Correct spelling mistakes in comments
Changed "avaiable" to "available" and "interupt" to "interrupt".
Signed-off-by: Jona Crasselt
Signed-off-by: Felix Windsheimer
---
drivers/staging/mt7621-mmc/sd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
index 149ea9ecfe32..ef3841604cfc 100644
--- a/drivers/staging/mt7621-mmc/sd.c
+++ b/drivers/staging/mt7621-mmc/sd.c
@@ -1445,7 +1445,7 @@ static void msdc_enable_cd_irq(struct msdc_host *host,
int enable)
/* for sdio, not set */
if ((hw->flags & MSDC_CD_PIN_EN) == 0) {
- /* Pull down card detection pin since it is not avaiable */
+ /* Pull down card detection pin since it is not available */
/*
* if (hw->config_gpio_pin)
* hw->config_gpio_pin(MSDC_CD_PIN, GPIO_PULL_DOWN);
@@ -1542,7 +1542,7 @@ static void msdc_init_hw(struct msdc_host *host)
/* Configure to enable SDIO mode. it's must otherwise sdio cmd5 failed
*/
sdr_set_bits(host->base + SDC_CFG, SDC_CFG_SDIO);
- /* disable detect SDIO device interupt function */
+ /* disable detect SDIO device interrupt function */
sdr_clr_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
/* eneable SMT for glitch filter */
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/7] mt7621-mmc: Fix some coding style issues
On Wed, Dec 19, 2018 at 05:07:01PM +0100, Jona Crasselt wrote: > Fix some comment-related issues reported by checkpatch.pl: > - Block comments use * on subsequent lines > - Block comments use a trailing */ on a separate line > > Signed-off-by: Jona Crasselt > Signed-off-by: Felix Windsheimer > --- > drivers/staging/mt7621-mmc/sd.c | 31 +-- > 1 file changed, 17 insertions(+), 14 deletions(-) Nice, but next time please put "staging: " at the front of your subject line for all of these patches. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 7/7] mt7621-mmc: Correct spelling mistakes in comments
On Wed, Dec 19, 2018 at 05:07:07PM +0100, Jona Crasselt wrote: > Signed-off-by: Jona Crasselt > Signed-off-by: Felix Windsheimer > --- > drivers/staging/mt7621-mmc/sd.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) I can't take patches without any changelog comments at all :( ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 6/7] mt7621-mmc: printk() should include KERN_ facility level
On Wed, Dec 19, 2018 at 05:07:06PM +0100, Jona Crasselt wrote:
> Issue reported by checkpatch.pl.
> Replace printk() with pr_debug() to include facility level.
>
> Signed-off-by: Jona Crasselt
> Signed-off-by: Felix Windsheimer
> ---
> drivers/staging/mt7621-mmc/sd.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
> index 357418999b1f..149ea9ecfe32 100644
> --- a/drivers/staging/mt7621-mmc/sd.c
> +++ b/drivers/staging/mt7621-mmc/sd.c
> @@ -1238,10 +1238,10 @@ static void msdc_ops_set_ios(struct mmc_host *mmc,
> struct mmc_ios *ios)
> "LEGACY", "MMC_HS", "SD_HS"
> };
>
> - printk("SET_IOS: CLK(%dkHz), BUS(%s), BW(%u), PWR(%s), VDD(%s),
> TIMING(%s)",
> - ios->clock / 1000, bus_mode[ios->bus_mode],
> - (ios->bus_width == MMC_BUS_WIDTH_4) ? 4 : 1,
> - power_mode[ios->power_mode], vdd[ios->vdd],
> timing[ios->timing]);
> + pr_debug("SET_IOS: CLK(%dkHz), BUS(%s), BW(%u), PWR(%s), VDD(%s),
> TIMING(%s)",
> + ios->clock / 1000, bus_mode[ios->bus_mode],
> + (ios->bus_width == MMC_BUS_WIDTH_4) ? 4 : 1,
> + power_mode[ios->power_mode], vdd[ios->vdd],
> timing[ios->timing]);
How do you know this is a "debug" message?
And as you have a struct device somewhere around here, please use
dev_dbg() instead of pr_debug(), it provides more information to the
developer.
thanks,
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFCv2 0/4] mm/memory_hotplug: Introduce memory block types
On 20.12.18 14:08, Michal Hocko wrote:
> On Thu 20-12-18 13:58:16, David Hildenbrand wrote:
>> On 30.11.18 18:59, David Hildenbrand wrote:
>>> This is the second approach, introducing more meaningful memory block
>>> types and not changing online behavior in the kernel. It is based on
>>> latest linux-next.
>>>
>>> As we found out during dicussion, user space should always handle onlining
>>> of memory, in any case. However in order to make smart decisions in user
>>> space about if and how to online memory, we have to export more information
>>> about memory blocks. This way, we can formulate rules in user space.
>>>
>>> One such information is the type of memory block we are talking about.
>>> This helps to answer some questions like:
>>> - Does this memory block belong to a DIMM?
>>> - Can this DIMM theoretically ever be unplugged again?
>>> - Was this memory added by a balloon driver that will rely on balloon
>>> inflation to remove chunks of that memory again? Which zone is advised?
>>> - Is this special standby memory on s390x that is usually not automatically
>>> onlined?
>>>
>>> And in short it helps to answer to some extend (excluding zone imbalances)
>>> - Should I online this memory block?
>>> - To which zone should I online this memory block?
>>> ... of course special use cases will result in different anwers. But that's
>>> why user space has control of onlining memory.
>>>
>>> More details can be found in Patch 1 and Patch 3.
>>> Tested on x86 with hotplugged DIMMs. Cross-compiled for PPC and s390x.
>>>
>>>
>>> Example:
>>> $ udevadm info -q all -a /sys/devices/system/memory/memory0
>>> KERNEL=="memory0"
>>> SUBSYSTEM=="memory"
>>> DRIVER==""
>>> ATTR{online}=="1"
>>> ATTR{phys_device}=="0"
>>> ATTR{phys_index}==""
>>> ATTR{removable}=="0"
>>> ATTR{state}=="online"
>>> ATTR{type}=="boot"
>>> ATTR{valid_zones}=="none"
>>> $ udevadm info -q all -a /sys/devices/system/memory/memory90
>>> KERNEL=="memory90"
>>> SUBSYSTEM=="memory"
>>> DRIVER==""
>>> ATTR{online}=="1"
>>> ATTR{phys_device}=="0"
>>> ATTR{phys_index}=="005a"
>>> ATTR{removable}=="1"
>>> ATTR{state}=="online"
>>> ATTR{type}=="dimm"
>>> ATTR{valid_zones}=="Normal"
>>>
>>>
>>> RFC -> RFCv2:
>>> - Now also taking care of PPC (somehow missed it :/ )
>>> - Split the series up to some degree (some ideas on how to split up patch 3
>>> would be very welcome)
>>> - Introduce more memory block types. Turns out abstracting too much was
>>> rather confusing and not helpful. Properly document them.
>>>
>>> Notes:
>>> - I wanted to convert the enum of types into a named enum but this
>>> provoked all kinds of different errors. For now, I am doing it just like
>>> the other types (e.g. online_type) we are using in that context.
>>> - The "removable" property should never have been named like that. It
>>> should have been "offlinable". Can we still rename that? E.g. boot memory
>>> is sometimes marked as removable ...
>>>
>>
>>
>> Any feedback regarding the suggested block types would be very much
>> appreciated!
>
> I still do not like this much to be honest. I just didn't get to think
> through this properly. My fear is that this is conflating an actual API
> with the current implementation and as such will cause problems in
> future. But I haven't really looked into your patches closely so I might
> be wrong. Anyway I won't be able to look into it by the end of year.
>
I guess as long as we have memory block devices and we expect user space
to make a decision we will have this API and the involved problems.
I am open for alternatives, and as I said, any feedback on how to sort
this out will be highly appreciated.
I'll be on vacation for the next two weeks, so this can wait. Just
wanted to note that I am still interested in feedback :)
--
Thanks,
David / dhildenb
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFCv2 0/4] mm/memory_hotplug: Introduce memory block types
On Thu 20-12-18 13:58:16, David Hildenbrand wrote:
> On 30.11.18 18:59, David Hildenbrand wrote:
> > This is the second approach, introducing more meaningful memory block
> > types and not changing online behavior in the kernel. It is based on
> > latest linux-next.
> >
> > As we found out during dicussion, user space should always handle onlining
> > of memory, in any case. However in order to make smart decisions in user
> > space about if and how to online memory, we have to export more information
> > about memory blocks. This way, we can formulate rules in user space.
> >
> > One such information is the type of memory block we are talking about.
> > This helps to answer some questions like:
> > - Does this memory block belong to a DIMM?
> > - Can this DIMM theoretically ever be unplugged again?
> > - Was this memory added by a balloon driver that will rely on balloon
> > inflation to remove chunks of that memory again? Which zone is advised?
> > - Is this special standby memory on s390x that is usually not automatically
> > onlined?
> >
> > And in short it helps to answer to some extend (excluding zone imbalances)
> > - Should I online this memory block?
> > - To which zone should I online this memory block?
> > ... of course special use cases will result in different anwers. But that's
> > why user space has control of onlining memory.
> >
> > More details can be found in Patch 1 and Patch 3.
> > Tested on x86 with hotplugged DIMMs. Cross-compiled for PPC and s390x.
> >
> >
> > Example:
> > $ udevadm info -q all -a /sys/devices/system/memory/memory0
> > KERNEL=="memory0"
> > SUBSYSTEM=="memory"
> > DRIVER==""
> > ATTR{online}=="1"
> > ATTR{phys_device}=="0"
> > ATTR{phys_index}==""
> > ATTR{removable}=="0"
> > ATTR{state}=="online"
> > ATTR{type}=="boot"
> > ATTR{valid_zones}=="none"
> > $ udevadm info -q all -a /sys/devices/system/memory/memory90
> > KERNEL=="memory90"
> > SUBSYSTEM=="memory"
> > DRIVER==""
> > ATTR{online}=="1"
> > ATTR{phys_device}=="0"
> > ATTR{phys_index}=="005a"
> > ATTR{removable}=="1"
> > ATTR{state}=="online"
> > ATTR{type}=="dimm"
> > ATTR{valid_zones}=="Normal"
> >
> >
> > RFC -> RFCv2:
> > - Now also taking care of PPC (somehow missed it :/ )
> > - Split the series up to some degree (some ideas on how to split up patch 3
> > would be very welcome)
> > - Introduce more memory block types. Turns out abstracting too much was
> > rather confusing and not helpful. Properly document them.
> >
> > Notes:
> > - I wanted to convert the enum of types into a named enum but this
> > provoked all kinds of different errors. For now, I am doing it just like
> > the other types (e.g. online_type) we are using in that context.
> > - The "removable" property should never have been named like that. It
> > should have been "offlinable". Can we still rename that? E.g. boot memory
> > is sometimes marked as removable ...
> >
>
>
> Any feedback regarding the suggested block types would be very much
> appreciated!
I still do not like this much to be honest. I just didn't get to think
through this properly. My fear is that this is conflating an actual API
with the current implementation and as such will cause problems in
future. But I haven't really looked into your patches closely so I might
be wrong. Anyway I won't be able to look into it by the end of year.
--
Michal Hocko
SUSE Labs
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFCv2 0/4] mm/memory_hotplug: Introduce memory block types
On 30.11.18 18:59, David Hildenbrand wrote:
> This is the second approach, introducing more meaningful memory block
> types and not changing online behavior in the kernel. It is based on
> latest linux-next.
>
> As we found out during dicussion, user space should always handle onlining
> of memory, in any case. However in order to make smart decisions in user
> space about if and how to online memory, we have to export more information
> about memory blocks. This way, we can formulate rules in user space.
>
> One such information is the type of memory block we are talking about.
> This helps to answer some questions like:
> - Does this memory block belong to a DIMM?
> - Can this DIMM theoretically ever be unplugged again?
> - Was this memory added by a balloon driver that will rely on balloon
> inflation to remove chunks of that memory again? Which zone is advised?
> - Is this special standby memory on s390x that is usually not automatically
> onlined?
>
> And in short it helps to answer to some extend (excluding zone imbalances)
> - Should I online this memory block?
> - To which zone should I online this memory block?
> ... of course special use cases will result in different anwers. But that's
> why user space has control of onlining memory.
>
> More details can be found in Patch 1 and Patch 3.
> Tested on x86 with hotplugged DIMMs. Cross-compiled for PPC and s390x.
>
>
> Example:
> $ udevadm info -q all -a /sys/devices/system/memory/memory0
> KERNEL=="memory0"
> SUBSYSTEM=="memory"
> DRIVER==""
> ATTR{online}=="1"
> ATTR{phys_device}=="0"
> ATTR{phys_index}==""
> ATTR{removable}=="0"
> ATTR{state}=="online"
> ATTR{type}=="boot"
> ATTR{valid_zones}=="none"
> $ udevadm info -q all -a /sys/devices/system/memory/memory90
> KERNEL=="memory90"
> SUBSYSTEM=="memory"
> DRIVER==""
> ATTR{online}=="1"
> ATTR{phys_device}=="0"
> ATTR{phys_index}=="005a"
> ATTR{removable}=="1"
> ATTR{state}=="online"
> ATTR{type}=="dimm"
> ATTR{valid_zones}=="Normal"
>
>
> RFC -> RFCv2:
> - Now also taking care of PPC (somehow missed it :/ )
> - Split the series up to some degree (some ideas on how to split up patch 3
> would be very welcome)
> - Introduce more memory block types. Turns out abstracting too much was
> rather confusing and not helpful. Properly document them.
>
> Notes:
> - I wanted to convert the enum of types into a named enum but this
> provoked all kinds of different errors. For now, I am doing it just like
> the other types (e.g. online_type) we are using in that context.
> - The "removable" property should never have been named like that. It
> should have been "offlinable". Can we still rename that? E.g. boot memory
> is sometimes marked as removable ...
>
Any feedback regarding the suggested block types would be very much
appreciated!
--
Thanks,
David / dhildenb
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] rts5208: add a missing check for the status of command sending
I think maybe this is the first kernel patch I have recieved from you. When you're adding error handling there are a couple ways to go wrong and this is what I look at when I review error handling patches: 1) The error handling is not required. 2) The error handling is not complete. I have messed up on both of these in my own patches because sometimes the code is complicated to understand. Sometimes there isn't any way to recover from errors. For example, we sometimes deliberately assume that the PCI bus is working because if it's not the real fix is to buy new hardware. Another thing that helps is to try write about the real world impact about the patch in the changelog. Try ask, why has that bug not been found before? Always take a look at how the function is called and the wider context. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] rts5208: add a missing check for the status of command sending
On Thu, Dec 20, 2018 at 01:57:01AM -0600, Kangjie Lu wrote: > ms_send_cmd() may fail. The fix checks the return value of it, and if it > fails, returns the error "STATUS_FAIL" upstream. > > Signed-off-by: Kangjie Lu > --- > drivers/staging/rts5208/ms.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/rts5208/ms.c b/drivers/staging/rts5208/ms.c > index f53adf15c685..5a9e562465e9 100644 > --- a/drivers/staging/rts5208/ms.c > +++ b/drivers/staging/rts5208/ms.c > @@ -2731,7 +2731,9 @@ static int mspro_rw_multi_sector(struct scsi_cmnd *srb, > } > > if (val & MS_INT_BREQ) You would need to add a curly brace here. > - ms_send_cmd(chip, PRO_STOP, WAIT_INT); > + retval = ms_send_cmd(chip, PRO_STOP, WAIT_INT); > + if (retval != STATUS_SUCCESS) > + return STATUS_FAIL; You can't overwrite "retval". We already had an error code save there but now you're changing to STATUS_SUCCESS. Anyway, I think the original error handling is probably correct and we should leave it as-is. We're already trying to handle an error situation by resetting stuff. Then if we get another error while we take these extreme measures to recover, we shouldn't give up. We should just keep on trying to reset instead of returning early. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
