On Wed, 2024-08-14 at 20:32 +0300, Sergey Shtylyov wrote:
> 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...
Ah, typo. pcim_iomap_regionS() is being replaced.
>
> > Remove all calls to pcim_iomap_table().
> >
> > Signed-off-by: Philipp Stanner <pstanner@xxxxxxxxxx>
> [...]
> > 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.
Thx
>
> [...]
> > 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?
I mean, this would at least make it perfectly readable what's being
done.
I guess we could do something like this, maybe with a comment explining
what is going on:
for_each_set_bit(j, 0x3 << base, PCI_STD_NUM_BARS) {
host->iomap[j] = pcim_iomap_region(pdev, j, drv_name);
if (IS_ERR(host->iomap[j])) {
rc = PTR_ERR(host->iomap[j]);
break;
}
}
if (rc) {
dev_warn(gdev,
Unless you've got a better idea?
Tell me which version you'd prefer.
>
> [...]
> > 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...
ACK, will provide better naming in v2.
Regards,
P.
>
> [...]
>
> MBR, Sergey
>
