Re: [PATCH v2] rpmsg: fix possible refcount leak in rpmsg_register_device_override()

From: Krzysztof Kozlowski
Date: Wed Jun 29 2022 - 04:02:12 EST


On 28/06/2022 18:31, Mathieu Poirier wrote:
> On Sat, Jun 25, 2022 at 09:40:36PM +0200, Krzysztof Kozlowski wrote:
>> On 24/06/2022 20:43, Mathieu Poirier wrote:
>>> On Fri, 24 Jun 2022 at 11:45, Krzysztof Kozlowski
>>> <krzysztof.kozlowski@xxxxxxxxxx> wrote:
>>>>
>>>> On 24/06/2022 19:36, Mathieu Poirier wrote:
>>>>> On Fri, Jun 24, 2022 at 10:41:20AM +0800, Hangyu Hua wrote:
>>>>>> rpmsg_register_device_override need to call put_device to free vch when
>>>>>> driver_set_override fails.
>>>>>>
>>>>>> Fix this by adding a put_device() to the error path.
>>>>>>
>>>>>> Fixes: bb17d110cbf2 ("rpmsg: Fix calling device_lock() on non-initialized device")
>>>>>
>>>>> This is funny... Neither Bjorn nor I have reviewed this patch...
>>>>
>>>> It was a fix for commit in Greg's tree and Greg's pick it up after a
>>>> week or something. I am not sure if that's actually funny that Greg has
>>>> to pick it up without review :(
>>>>
>>>
>>> The patch was sent out on April 19th and committed 3 days later on
>>> April 22nd. Is 3 days the new patch review time standard?
>>
>> Neither 19th, nor 22nd are correct. The patch which you set you never
>> reviewed, so commit bb17d110cbf2 was sent on 29th of April:
>> https://lore.kernel.org/all/20220429195946.1061725-1-krzysztof.kozlowski@xxxxxxxxxx/
>>
>
> Twitchy fingers... Those dates are for commit 42cd402b8fd4, which is referenced
> by bb17d110cbf2.
>
> The end result is the same, that is patches related to the remoteproc/rpmsg
> subsystems (or any subsystem) should not be committed before their maintainers
> have the opportunity to review them.

I think two months for your involvement was enough. During this two
months there was a comment from Bjorn, applied and later quite plenty of
time to revise/check/review.

I understand that we are all busy, pretty often working on upstream in
spare time etc. So no one here complained that you did not review this
patch within two months. But I don't understand what shall we do more if
two months are not enough - wait four months? Six months?

Best regards,
Krzysztof