linux-kernel - Re: [PATCH 6/6] (v3) drivers: hwmon: i5k_amb: simplify probing / device identification

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] [day] [month] [year] [list]

Message-ID: <07148b8e-5b87-66ad-e52b-be2b8b22e712@metux.net>
Date:   Wed, 8 Jan 2020 11:16:47 +0100
From:   "Enrico Weigelt, metux IT consult" <lkml@...ux.net>
To:     Bjorn Helgaas <helgaas@...nel.org>,
        Guenter Roeck <linux@...ck-us.net>
Cc:     "Enrico Weigelt, metux IT consult" <info@...ux.net>,
        linux-kernel@...r.kernel.org, tim@...tersideup.com,
        james.morse@....com, rrichter@...vell.com, jdelvare@...e.com,
        miquel.raynal@...tlin.com, richard@....at, vigneshr@...com,
        linux-crypto@...r.kernel.org, linux-edac@...r.kernel.org,
        linux-hwmon@...r.kernel.org, linux-mtd@...ts.infradead.org,
        linux-pci@...r.kernel.org
Subject: Re: [PATCH 6/6] (v3) drivers: hwmon: i5k_amb: simplify probing /
 device identification

On 11.12.19 00:25, Bjorn Helgaas wrote:

Hi,

>   2) I despise the pci_get_device() interfaces because they're
>   inefficient, not hotplug-safe, they circumvent the device model
>   claim mechanism, and it's hard to do the reference counting
>   correctly.

Agreed, but for that we'd have to edac and hwmo driver together into
one (or maybe a MFD). That would be a major rewrite, and I don't have
the necessary HW to test it.

>   3) There are several things going on in this patch and it would be
>   easier to read if you could split them into separate patches:
> 
>     - Removing the redundancy between chipset_ids[] and i5k_amb_ids[].
>       This seems like a nice change.
> 
>     - The "chipset_ids[i].fbd0 + 1" thing was weird and the new
>       ".driver_data + 1" is still weird.  Those are PCI device IDs,
>       and addition is not a valid operation on those IDs.  IMHO both
>       PCI_DEVICE_ID_INTEL_5000_FBD0 and PCI_DEVICE_ID_INTEL_5000_FBD1
>       should be listed explicitly in the driver instead of trying to
>       compute PCI_DEVICE_ID_INTEL_5000_FBD1.

Ok, good point.

>     - Replacing the hard-coding of PCI_VENDOR_ID_INTEL with the vendor
>       ID from i5k_amb_ids[] seems worthwhile and should be its own
>       separate patch (if possible).
> 
>     - Changing to use pci_get_device_by_id().  This should be trivial
>       to verify, like the other patches.

Ok, I'll try to split it up more clearly, once I've got some more spare
time.


--mtx

---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@...ux.net -- +49-151-27565287