linux-kernel - Re: [PATCH v3 8/9] drm/tegra: vic: Implement get_streamid

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 22 Feb 2022 12:54:42 +0200
From:   Mikko Perttunen <cyndis@...si.fi>
To:     Dmitry Osipenko <digetx@...il.com>,
        Mikko Perttunen <mperttunen@...dia.com>,
        thierry.reding@...il.com, jonathanh@...dia.com, joro@...tes.org,
        will@...nel.org, robh+dt@...nel.org, robin.murphy@....com
Cc:     linux-tegra@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        iommu@...ts.linux-foundation.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 8/9] drm/tegra: vic: Implement get_streamid_offset

On 2/22/22 12:46, Dmitry Osipenko wrote:
> 22.02.2022 11:27, Mikko Perttunen пишет:
>> On 2/21/22 22:10, Dmitry Osipenko wrote:
>>> 21.02.2022 14:44, Mikko Perttunen пишет:
>>>> On 2/19/22 20:54, Dmitry Osipenko wrote:
>>>>> 19.02.2022 21:49, Dmitry Osipenko пишет:
>>>>>> 18.02.2022 14:39, Mikko Perttunen пишет:
>>>>>>> +static int vic_get_streamid_offset(struct tegra_drm_client *client)
>>>>>>> +{
>>>>>>> +    struct vic *vic = to_vic(client);
>>>>>>> +    int err;
>>>>>>> +
>>>>>>> +    err = vic_load_firmware(vic);
>>>>>>
>>>>>> You can't invoke vic_load_firmware() while RPM is suspended. Either
>>>>>> replace this with RPM get/put or do something else.
>>>>
>>>> Why not, I'm not seeing any HW accesses in vic_load_firmware? Although
>>>> it looks like it might race with the vic_load_firmware call in
>>>> vic_runtime_resume which probably needs to be fixed.
>>>
>>> It was not clear from the function's name that h/w is untouched, I read
>>> "load" as "upload" and then looked at vic_runtime_resume(). I'd rename
>>> vic_load_firmware() to vic_prepare_firmware_image().
>>>
>>> And yes, technically lock is needed.
>>
>> Yep, I'll consider renaming it.
> 
> Looking at this all again, I'd suggest to change:
> 
> int get_streamid_offset(client)
> 
> to:
> 
> int get_streamid_offset(client, *offset)
> 
> and bail out if get_streamid_offset() returns error. It's never okay to
> ignore errors.

Sure, seems reasonable. We'll still need some error code to indicate 
that context isolation isn't available for the engine and continue on in 
that case but that's better than just ignoring all of them.

Mikko