Re: [PATCH] ata: Replace deprecated PCI devres functions

[Philipp Stanner <pstanner@redhat.com>]

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...
> 
> > Remove all calls to pcim_iomap_table().
> > 
> > Signed-off-by: Philipp Stanner <pstanner@redhat.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?

Oh, forgot to address this.

Yes, it does ignore it, but it behaves as the existing code does. The
existing version jumps into ata_pci_bmdma_init_one() if it cannot
request or ioremap the BAR.

	/* 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)
		goto use_ioports;


Is that a bug in the existing version, too?
The comment hints to me that this is fine and intended.

Otherwise we want to remain consistent with the pre-existing behavior.


P.

> 
> [...]
> > 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
>