linux-kernel - Re: [PATCH] drivers: block: skd: remove skd_pci

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:   Sat, 12 Dec 2020 01:59:51 +0000
From:   Chaitanya Kulkarni <Chaitanya.Kulkarni@....com>
To:     Bjorn Helgaas <helgaas@...nel.org>
CC:     Puranjay Mohan <puranjay12@...il.com>,
        "bjorn@...gaas.com" <bjorn@...gaas.com>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        Damien Le Moal <Damien.LeMoal@....com>,
        "axboe@...nel.dk" <axboe@...nel.dk>,
        "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drivers: block: skd: remove skd_pci_info()

On 12/11/20 14:41, Bjorn Helgaas wrote:
>> The skd driver prints unknown if the speed is not "2.5GT/s" or "5.0GT/s".
>> __pcie_print_link_status()  prints "unknown" only if speed
>> value >= ARRAY_SIZE(speed_strings).
>>
>> If a buggy skd card returns value that is not != ("2.5GT/s" or "5.0GT/s")
>> && value < ARRAY_SIZE(speed_strings) then it will not print the unknown but
>> the value from speed string array.
>>
>> Which breaks the current behavior. Please correct me if I'm wrong.
> I think you're right, but I don't think it actually *breaks* anything.
>
> For skd devices that work correctly, there should be no problem, and
> if there ever were an skd device that operated at a speed greater than
> 5GT/s, the PCI core would print that speed correctly instead of having
> the driver print "<unknown>".
That is the scenario I'm not aware why it prints unknown to start with
and I couldn't find any documentation also, that is why the concern.
> I don't think it's a good idea to have a driver artificially constrain
> the speed a device operates at.  And the existing code doesn't
> actually constrain anything; it only prints "<unknown>" if it doesn't
> recognize it.  The probe still succeeds.  I don't see much value in
> that "<unknown>".
>
> Or am I missing an actual problem this patch causes?
You are not, I'm just not sure without any documentation why does
it print "unknown" and I attributed that to probable firmware issue
(since we all knowhow creative firmware can get ;)).

That makes it the problem with original code more so than with this patch.
In that case I was proposing just keep the original behavior.

But maybe we should apply patch and if any user(s) comes up with the problem
then we can deal with it.

Whoever is going to apply they can add :-

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@....com>