greybus-dev January 2022

greybus-dev@lists.linaro.org

11 participants
6 discussions

[PATCH v2 0/7] Provide and use generic_handle_irq_safe() where appropriate.

by Sebastian Andrzej Siewior

generic_handle_irq() must be used from primary IRQ-handler (chain handler/ interrupt controller entry). It is low level code and the function expects that interrupts are disabled at entry point. This isn't the case for invocations from tasklets, workqueues or the primary interrupt handler on PREEMPT_RT. Once this gets noticed a "local_irq_disable|safe()" is added. To avoid further confusion this series adds generic_handle_irq_safe() which can be used from any context and adds a few user. v2…v1: - Redo kernel-doc for generic_handle_irq_safe() in #1. - Use generic_handle_irq_safe() instead of generic_handle_irq() in the patch description where I accidently used the wrong one. v1: https://lore.kernel.org/all/20220127113303.3012207-1-bigeasy@linutronix.de/ Sebastian

2 years, 7 months

[PATCH 0/7] Provide and use generic_handle_irq_safe() where appropriate.

by Sebastian Andrzej Siewior

2 years, 7 months

[PATCH][next] staging: greybus: i2c: Use struct_size() helper in gb_i2c_operation_create()

by Gustavo A. R. Silva

Make use of the struct_size() helper instead of an open-coded version, in order to avoid any potential type mistakes or integer overflows that, in the worst scenario, could lead to heap overflows. Also, address the following sparse warnings: drivers/staging/greybus/i2c.c:111:24: warning: using sizeof on a flexible structure Link: https://github.com/KSPP/linux/issues/174 Signed-off-by: Gustavo A. R. Silva <gustavoars(a)kernel.org> --- drivers/staging/greybus/i2c.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/staging/greybus/i2c.c b/drivers/staging/greybus/i2c.c index de2f6516da09..9dfc6791c20e 100644 --- a/drivers/staging/greybus/i2c.c +++ b/drivers/staging/greybus/i2c.c @@ -108,9 +108,7 @@ gb_i2c_operation_create(struct gb_connection *connection, else data_out_size += (u32)msg->len; - request_size = sizeof(*request); - request_size += msg_count * sizeof(*op); - request_size += data_out_size; + request_size = struct_size(request, ops, msg_count) + data_out_size; /* Response consists only of incoming data */ operation = gb_operation_create(connection, GB_I2C_TYPE_TRANSFER, -- 2.27.0

2 years, 7 months

Re: [greybus-dev] [PATCH] staging: greybus: audio: Check null pointer

by Alex Elder

On 12/27/21 8:02 PM, Jiasheng Jiang wrote: > Sorry the previous email is forgetten to wrap line. > This email is corrected and the content is the same. > > On Mon, Dec 27, 2021 at 11:54:10PM +0800, Alex Elder wrote: >> I think this is a good change, but I would like you to improve >> the description, and fix some different bugs introduced by your >> change. >> >> What you are specifically doing is checking for a null return >> from devm_kcalloc() in gb_generate_enum_strings(), and are >> returning the NULL pointer if that occurs. That means you >> need to update all the callers of gb_generate_enum_strings() >> to also handle a possible null return value. >> >> The fix does a good thing, and your description is correct >> about what you are fixing. But it should supply more >> complete context for the change. > > Thanks for your advice, I will correct my description in next version. > But I still have some question about the devm_kzalloc(). > >> You can't simply return here. If you look a bit above this, >> where the call to allocate a control structure is done, you >> see that a NULL return there jumps to the "error" label, so >> any already allocated and initialized control widgets get >> cleaned up before returning. > > Actually, I have already thought of whether it needs to free after the > devm_kzalloc(). > As we can find in the gbaudio_tplg_create_widget(), the widget_kctls is > allocated by devm_kzalloc(), but isn't released when > gbaudio_tplg_create_wcontrol() fails and goto error. > And I check of the comment of the devm_kmalloc() in `drivers/base/devres.c`, > because devm_kzalloc() returns devm_kmalloc(). > And it says that "Memory allocated with this function is automatically > freed on driver detach." > So there is no need to free the memory manually. > Is that right? You are partially right, but you're missing something. Yes, anything allocated with devm_kzalloc() will be freed automatically when the last reference to the device is dropped. But the two places you're returning are in the middle of a loop (in gbaudio_tplg_create_widget() and gbaudio_tplg_process_kcontrols()). Each is building up a sort of hierarchical data structure, and as you can see, the existing code takes care to de-construct the partially built structure in the event an error occurs before it completes. There is a chance that your simple return would "work", but by reading the surrounding code you should recognize that the proper way to handle the error is to jump to the cleanup at the end. The other alternative would be to fix those functions so they don't do that controlled cleanup on error and simply return early (as you were proposing). But without digging deeper, I would assume the original developers designed it this way very intentionally, because it avoids a problem somewhere. -Alex > And I am sorry again because of the lack of the above explanation in my > commit message. > I will also add to my new commit. > > Thanks, > Jiang >

2 years, 8 months

Re: [PATCH v3] staging: greybus: audio: Check null pointer

by Alex Elder

On 1/4/22 9:06 AM, Jiasheng Jiang wrote: > As the possible alloc failure of devm_kcalloc(), it could return null > pointer. > Therefore, 'strings' should be checked and return NULL if alloc fails to > prevent the dereference of the NULL pointer. > Also, the caller should also deal with the return value of the > gb_generate_enum_strings() and return -ENOMEM if returns NULL. > Moreover, because the memory allocated with devm_kzalloc() will be > freed automatically when the last reference to the device is dropped, > the 'gbe' in gbaudio_tplg_create_enum_kctl() and > gbaudio_tplg_create_enum_ctl() do not need to free manually. > But the 'control' in gbaudio_tplg_create_widget() and > gbaudio_tplg_process_kcontrols() has a specially error handle to > cleanup. > So it should be better to cleanup 'control' when fails. > > Fixes: e65579e335da ("greybus: audio: topology: Enable enumerated control support") > Signed-off-by: Jiasheng Jiang <jiasheng(a)iscas.ac.cn> This looks good to me. Thanks for fixing things. Reviewed-by: Alex Elder <elder(a)linaro.org> > --- > v3: Same code as v2, but remove the redundant message as requested. > v2: Updated to use common error processing at the end of both > gbaudio_tplg_create_widget() and gbaudio_tplg_process_kcontrols(). > --- > drivers/staging/greybus/audio_topology.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c > index 1fc7727ab7be..6bba735d2e5c 100644 > --- a/drivers/staging/greybus/audio_topology.c > +++ b/drivers/staging/greybus/audio_topology.c > @@ -147,6 +147,9 @@ static const char **gb_generate_enum_strings(struct gbaudio_module_info *gb, > > items = le32_to_cpu(gbenum->items); > strings = devm_kcalloc(gb->dev, items, sizeof(char *), GFP_KERNEL); > + if (!strings) > + return NULL; > + > data = gbenum->names; > > for (i = 0; i < items; i++) { > @@ -655,6 +658,8 @@ static int gbaudio_tplg_create_enum_kctl(struct gbaudio_module_info *gb, > /* since count=1, and reg is dummy */ > gbe->items = le32_to_cpu(gb_enum->items); > gbe->texts = gb_generate_enum_strings(gb, gb_enum); > + if (!gbe->texts) > + return -ENOMEM; > > /* debug enum info */ > dev_dbg(gb->dev, "Max:%d, name_length:%d\n", gbe->items, > @@ -862,6 +867,8 @@ static int gbaudio_tplg_create_enum_ctl(struct gbaudio_module_info *gb, > /* since count=1, and reg is dummy */ > gbe->items = le32_to_cpu(gb_enum->items); > gbe->texts = gb_generate_enum_strings(gb, gb_enum); > + if (!gbe->texts) > + return -ENOMEM; > > /* debug enum info */ > dev_dbg(gb->dev, "Max:%d, name_length:%d\n", gbe->items, > @@ -1034,6 +1041,10 @@ static int gbaudio_tplg_create_widget(struct gbaudio_module_info *module, > csize += le16_to_cpu(gbenum->names_length); > control->texts = (const char * const *) > gb_generate_enum_strings(module, gbenum); > + if (!control->texts) { > + ret = -ENOMEM; > + goto error; > + } > control->items = le32_to_cpu(gbenum->items); > } else { > csize = sizeof(struct gb_audio_control); > @@ -1183,6 +1194,10 @@ static int gbaudio_tplg_process_kcontrols(struct gbaudio_module_info *module, > csize += le16_to_cpu(gbenum->names_length); > control->texts = (const char * const *) > gb_generate_enum_strings(module, gbenum); > + if (!control->texts) { > + ret = -ENOMEM; > + goto error; > + } > control->items = le32_to_cpu(gbenum->items); > } else { > csize = sizeof(struct gb_audio_control); >

2 years, 8 months

Re: [PATCH v2] staging: greybus: audio: Check null pointer

by Alex Elder

On 12/28/21 8:28 PM, Jiasheng Jiang wrote: > On Tue, Dec 28, 2021 at 10:50:22PM +0800, Alex Elder wrote: >> But the two places you're returning are in the middle of a loop (in >> gbaudio_tplg_create_widget() and gbaudio_tplg_process_kcontrols()). >> Each is building up a sort of hierarchical data structure, and as >> you can see, the existing code takes care to de-construct the partially >> built structure in the event an error occurs before it completes. >> >> There is a chance that your simple return would "work", but by >> reading the surrounding code you should recognize that the proper >> way to handle the error is to jump to the cleanup at the end. > > Yes, I agree with your opinion and this time I use "goto error" instead > of directly return in the two callers. > The new patch is as follow. > > As the possible alloc failure of devm_kcalloc(), it could return null > pointer. > Therefore, 'strings' should be checked and return NULL if alloc fails to > prevent the dereference of the NULL pointer. > Also, the caller should also deal with the return value of the > gb_generate_enum_strings() and return -ENOMEM if returns NULL. > Moreover, because the memory allocated with devm_kzalloc() will be > freed automatically when the last reference to the device is dropped, > the 'gbe' in gbaudio_tplg_create_enum_kctl() and > gbaudio_tplg_create_enum_ctl() do not need to free manually. > But the 'control' in gbaudio_tplg_create_widget() and > gbaudio_tplg_process_kcontrols() has a specially error handle to > cleanup. > So it should be better to cleanup 'control' when fails. I haven't really looked at this yet, but in any case I would like you to send version 3 of this patch. The reason is that you should not include the message above in the patch itself. To be clear, I really appreciate your response; please *do* respond to review comments. But do so in e-mail only, and then follow up with a new patch, using the same basic subject (with a new version) and (roughly) the same description. So your patch should have a subject line with v3. It should then contain the original description (not indented with ">>" or anything), updated or improved if appropriate to reflect the current state of the patch. Then, below the "---" line you should include a patch history, with a line or two describing each version. For example, it might look something like: --- v3: Same code as v2, but fixed description as requested. v2: Updated to use common error processing at the end of both gbaudio_tplg_create_widget() and gbaudio_tplg_process_kcontrols(). I will review version 3, and if I don't see anything else wrong with it I'll offer a "Signed-off-by" tag. -Alex > Fixes: e65579e335da ("greybus: audio: topology: Enable enumerated control support") > Signed-off-by: Jiasheng Jiang <jiasheng(a)iscas.ac.cn> > --- > Changelog: > > v1 -> v2 > > *Change 1. Add the cleanup process when alloc fails in two callers and > refine the commit message. > --- > drivers/staging/greybus/audio_topology.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c > index 1fc7727ab7be..6bba735d2e5c 100644 > --- a/drivers/staging/greybus/audio_topology.c > +++ b/drivers/staging/greybus/audio_topology.c > @@ -147,6 +147,9 @@ static const char **gb_generate_enum_strings(struct gbaudio_module_info *gb, > > items = le32_to_cpu(gbenum->items); > strings = devm_kcalloc(gb->dev, items, sizeof(char *), GFP_KERNEL); > + if (!strings) > + return NULL; > + > data = gbenum->names; > > for (i = 0; i < items; i++) { > @@ -655,6 +658,8 @@ static int gbaudio_tplg_create_enum_kctl(struct gbaudio_module_info *gb, > /* since count=1, and reg is dummy */ > gbe->items = le32_to_cpu(gb_enum->items); > gbe->texts = gb_generate_enum_strings(gb, gb_enum); > + if (!gbe->texts) > + return -ENOMEM; > > /* debug enum info */ > dev_dbg(gb->dev, "Max:%d, name_length:%d\n", gbe->items, > @@ -862,6 +867,8 @@ static int gbaudio_tplg_create_enum_ctl(struct gbaudio_module_info *gb, > /* since count=1, and reg is dummy */ > gbe->items = le32_to_cpu(gb_enum->items); > gbe->texts = gb_generate_enum_strings(gb, gb_enum); > + if (!gbe->texts) > + return -ENOMEM; > > /* debug enum info */ > dev_dbg(gb->dev, "Max:%d, name_length:%d\n", gbe->items, > @@ -1034,6 +1041,10 @@ static int gbaudio_tplg_create_widget(struct gbaudio_module_info *module, > csize += le16_to_cpu(gbenum->names_length); > control->texts = (const char * const *) > gb_generate_enum_strings(module, gbenum); > + if (!control->texts) { > + ret = -ENOMEM; > + goto error; > + } > control->items = le32_to_cpu(gbenum->items); > } else { > csize = sizeof(struct gb_audio_control); > @@ -1183,6 +1194,10 @@ static int gbaudio_tplg_process_kcontrols(struct gbaudio_module_info *module, > csize += le16_to_cpu(gbenum->names_length); > control->texts = (const char * const *) > gb_generate_enum_strings(module, gbenum); > + if (!control->texts) { > + ret = -ENOMEM; > + goto error; > + } > control->items = le32_to_cpu(gbenum->items); > } else { > csize = sizeof(struct gb_audio_control); >

2 years, 8 months

2024

2023

2022

2021

2020

2019

2018

2017

2016

greybus-dev January 2022