On 2/22/22 12:19 AM, Song Chen wrote:
> Hi Greg,
>
> 在 2022/2/22 01:06, Greg KH 写道:
>> On Fri, Feb 11, 2022 at 08:02:27PM +0800, Song Chen wrote:
>>> Introduce apply in pwm_ops to replace legacy operations,
>>> like enable, disable, config and set_polarity.
>>>
>>> Signed-off-by: Song Chen <chensong_2000(a)189.cn>
. . .
>>> diff --git a/include/linux/greybus/greybus_protocols.h b/include/linux/greybus/greybus_protocols.h
>>> index aeb8f9243545..81a6f16de098 100644
>>> --- a/include/linux/greybus/greybus_protocols.h
>>> +++ b/include/linux/greybus/greybus_protocols.h
>>> @@ -812,8 +812,8 @@ struct gb_pwm_deactivate_request {
>>> struct gb_pwm_config_request {
>>> __u8 which;
>>> - __le32 duty;
>>> - __le32 period;
>>> + __u64 duty;
>>> + __u64 period;
>>> } __packed;
>>
>> Did you just change a greybus protocol message that is sent over the
>> wire? Why? And why drop the endian marking of it?
>
> I discussed with Uwe about losing bit and found there is no perfect
> way to avoid, even in Uwe's patch[1], as a result, we decided to
The patch you reference [1] does not change the size of the
duty cycle and period, it only ensures the value passed is
no more than can be represented in an integer.
> widen duty and period in gb_pwm_config_request, the other side of the
> wire is supposed to change accordingly to support 64bit duty and
This is what Greg meant about changing the over-the-wire protocol
message format. You can't do that, or rather, if you do that, it
is a *lot* more work than just changing it as you have done here.
If it is really required (and that's not clear, at least not at
this time), then you need to modify the protocol version, and
then make sure the versioning logic works correctly, both on
the AP side and on the module side for all existing modules.
I would suggest that it is *not* required though, because the
existing module code was built with the assumption that it would
be provided a 32-bit unsigned value for its duty cycle and period.
So as long as you make sure the AP side doesn't send anything
nonsensical, they will continue to work as desired.
The correct fix in this case (assuming you don't want to change
the protocol) is to ensure that whatever value is passed in to
the pwm_ops->config callback (which is gb_pwm_config()) is
adjusted to be within a range usable by the existing protocol.
Since it's actually an unsigned value, you could double the
range supported without changing the protocol if you wanted to
do that.
> period too(this will introduce compatibility problem and there is no
> variable like version to address it), similar with ktime_t in y2038,
> below is the detail [2]
And similar to addressing the Y2038 issue, it is not an easy thing to do.
> [1]:https://lore.kernel.org/all/20210312212119.1342666-1-u.kleine-koenig@pen…
> [2]:https://lore.kernel.org/all/20220211071601.4rpfbkit6c6dre2o@pengutronix.…
>
> endian is a problem, i shouldn't drop it.
This is absolutely correct. Did you attempt to compile this?
-Alex
> BR
>
> Song
>
>>
>> Are you sure this is ok?
>>
>> thanks,
>>
>> greg k-h
>>
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
Dan found and fixed a bug in the SVC HELLO request error handling which
was introduced when adding a temporary hack to reconfigure the link at
HELLO.
The implementation of the hack abuses the deferred-request processing
mechanism to send the link-configuration request, which makes the
HELLO processing a bit hard to follow.
The last two patches attempt to remedy this.
This could go into either 5.17-rc or -next.
Johan
Dan Carpenter (1):
greybus: svc: fix an error handling bug in gb_svc_hello()
Johan Hovold (2):
greybus: svc: clean up hello error path
greybus: svc: clean up link configuration hack at hello
drivers/greybus/svc.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
--
2.34.1
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.
Sebastian
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
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
>