| 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
| ||
|
Message-ID: <c2d21da0-7fe1-f995-5562-7ff04e9f1b8b@omp.ru>
Date: Wed, 14 Aug 2024 20:32:53 +0300
From: Sergey Shtylyov <s.shtylyov@....ru>
To: Philipp Stanner <pstanner@...hat.com>, Damien Le Moal
<dlemoal@...nel.org>, Niklas Cassel <cassel@...nel.org>, Mikael Pettersson
<mikpelinux@...il.com>
CC: <linux-ide@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ata: Replace deprecated PCI devres functions
On 8/12/24 11:48 AM, Philipp Stanner wrote:
> The ata subsystem uses the PCI devres functions pcim_iomap_table() and
> pcim_request_regions(), which have been deprecated in commit e354bb84a4c1
> ("PCI: Deprecate pcim_iomap_table(), pcim_iomap_regions_request_all()").
>
> These functions internally already use their successors, notably
> pcim_request_region(), so they are quite trivial to replace.
>
> However, one thing special about ata is that it stores the iomap table
> provided by pcim_iomap_table() in struct ata_host. This can be replaced
> with a __iomem pointer table, statically allocated with size
> PCI_STD_NUM_BARS so it can house the maximum number of PCI BARs. The
> only further modification then necessary is to explicitly fill that
> table, whereas before it was filled implicitly by
> pcim_request_regions().
>
> Modify the iomap table in struct ata_host.
>
> Replace all calls to pcim_request_region() with ones to
> pcim_request_region().
Huh? :-)
Besides, I'm not seeing pcim_request_region() anywhere in this patch...
> Remove all calls to pcim_iomap_table().
>
> Signed-off-by: Philipp Stanner <pstanner@...hat.com>
[...]
> drivers/ata/ata_piix.c | 7 +++---
> drivers/ata/libata-sff.c | 50 ++++++++++++++++++++++++++++++-------
> drivers/ata/pata_atp867x.c | 13 ++++++----
> drivers/ata/pata_hpt3x3.c | 8 +++---
> drivers/ata/pata_ninja32.c | 10 ++++----
> drivers/ata/pata_pdc2027x.c | 11 ++++----
> drivers/ata/pata_sil680.c | 11 ++++----
> drivers/ata/pdc_adma.c | 9 +++----
> drivers/ata/sata_inic162x.c | 10 +++-----
> drivers/ata/sata_mv.c | 8 +++---
> drivers/ata/sata_nv.c | 8 +++---
> drivers/ata/sata_promise.c | 7 +++---
> drivers/ata/sata_qstor.c | 7 +++---
> drivers/ata/sata_sil.c | 7 +++---
> drivers/ata/sata_sil24.c | 20 ++++++++-------
> drivers/ata/sata_sis.c | 8 +++---
> drivers/ata/sata_svw.c | 9 ++++---
> drivers/ata/sata_sx4.c | 17 ++++++++++---
> drivers/ata/sata_via.c | 31 ++++++++++++++---------
> drivers/ata/sata_vsc.c | 7 +++---
> include/linux/libata.h | 7 +++++-
> 21 files changed, 163 insertions(+), 102 deletions(-)
I did review all the changes, not just PATA drivers.
[...]
> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index 250f7dae05fd..d58db8226436 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
[...]
> @@ -2172,8 +2173,41 @@ int ata_pci_sff_init_host(struct ata_host *host)
> continue;
> }
>
> - rc = pcim_iomap_regions(pdev, 0x3 << base,
> - dev_driver_string(gdev));
> + /*
> + * In a first loop run, we want to get BARs 0 and 1.
> + * In a second run, we want BARs 2 and 3.
> + */
> + if (i == 0) {
> + io_tmp = pcim_iomap_region(pdev, 0, drv_name);
> + if (IS_ERR(io_tmp)) {
> + rc = PTR_ERR(io_tmp);
> + goto err;
> + }
> + host->iomap[0] = io_tmp;
> +
> + io_tmp = pcim_iomap_region(pdev, 1, drv_name);
> + if (IS_ERR(io_tmp)) {
> + rc = PTR_ERR(io_tmp);
> + goto err;
> + }
> + host->iomap[1] = io_tmp;
> + } else {
> + io_tmp = pcim_iomap_region(pdev, 2, drv_name);
> + if (IS_ERR(io_tmp)) {
> + rc = PTR_ERR(io_tmp);
> + goto err;
> + }
> + host->iomap[2] = io_tmp;
> +
> + io_tmp = pcim_iomap_region(pdev, 3, drv_name);
> + if (IS_ERR(io_tmp)) {
> + rc = PTR_ERR(io_tmp);
> + goto err;
> + }
> + host->iomap[3] = io_tmp;
> + }
> +
Ugh... Why you couldn't keep using base (or just i * 2) and avoid
such code duplication?
[...]
> diff --git a/drivers/ata/pata_sil680.c b/drivers/ata/pata_sil680.c
> index abe64b5f83cf..8a17df73412e 100644
> --- a/drivers/ata/pata_sil680.c
> +++ b/drivers/ata/pata_sil680.c
> @@ -360,15 +360,16 @@ static int sil680_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
> /* Try to acquire MMIO resources and fallback to PIO if
> * that fails
> */
> - rc = pcim_iomap_regions(pdev, 1 << SIL680_MMIO_BAR, DRV_NAME);
> - if (rc)
> + mmio_base = pcim_iomap_region(pdev, SIL680_MMIO_BAR, DRV_NAME);
> + if (IS_ERR(mmio_base)) {
> + rc = PTR_ERR(mmio_base);
goto use_ioports;
The code under that label ignores rc, no?
[...]
> diff --git a/drivers/ata/sata_sx4.c b/drivers/ata/sata_sx4.c
> index a482741eb181..d115f6f66974 100644
> --- a/drivers/ata/sata_sx4.c
> +++ b/drivers/ata/sata_sx4.c
> @@ -1390,6 +1390,7 @@ static int pdc_sata_init_one(struct pci_dev *pdev,
> struct ata_host *host;
> struct pdc_host_priv *hpriv;
> int i, rc;
> + void __iomem *io_tmp;
I'd suggest to call it base or s/th...
[...]
> diff --git a/drivers/ata/sata_via.c b/drivers/ata/sata_via.c
> index 57cbf2cef618..73b78834fa3f 100644
> --- a/drivers/ata/sata_via.c
> +++ b/drivers/ata/sata_via.c
> @@ -457,6 +457,7 @@ static int vt6420_prepare_host(struct pci_dev *pdev, struct ata_host **r_host)
> {
> const struct ata_port_info *ppi[] = { &vt6420_port_info, NULL };
> struct ata_host *host;
> + void __iomem *iomem;
Call it base, maybe?
[...]
> @@ -486,6 +488,7 @@ static int vt6421_prepare_host(struct pci_dev *pdev, struct ata_host **r_host)
> const struct ata_port_info *ppi[] =
> { &vt6421_sport_info, &vt6421_sport_info, &vt6421_pport_info };
> struct ata_host *host;
> + void __iomem *iomem;
Here as well...
[...]
MBR, Sergey
Powered by blists - more mailing lists