Received-SPF: pass (google.com: domain of linux-kernel+bounces-28297-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: andy.shevchenko@gmail.com
Date: Tue, 16 Jan 2024 23:27:53 +0200
To: Philipp Stanner <pstanner@redhat.com>
Cc: Jonathan Corbet <corbet@lwn.net>, Hans de Goede <hdegoede@redhat.com>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Maxime Ripard <mripard@kernel.org>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Sam Ravnborg <sam@ravnborg.org>, dakr@redhat.com,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	dri-devel@lists.freedesktop.org, linux-pci@vger.kernel.org
Subject: Re: [PATCH 02/10] pci: deprecate iomap-table functions
Message-ID: <Zab02RyfvP-IZTl4@surfacebook.localdomain>
References: <20240115144655.32046-2-pstanner@redhat.com>
 <20240115144655.32046-4-pstanner@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240115144655.32046-4-pstanner@redhat.com>

Mon, Jan 15, 2024 at 03:46:13PM +0100, Philipp Stanner kirjoitti:
> The old plural devres functions tie PCI's devres implementation to the
> iomap-table mechanism which unfortunately is not extensible.
> 
> As the purlal functions are almost never used with more than one bit set
> in their bit-mask, deprecating them and encouraging users to use the new
> singular functions instead is reasonable.
> 
> Furthermore, to make the implementation more consistent and extensible,
> the plural functions should use the singular functions.
> 
> Add new wrapper to request / release all BARs.
> Make the plural functions call into the singular functions.
> Mark the plural functions as deprecated.
> Remove as much of the iomap-table-mechanism as possible.
> Add comments describing the path towards a cleaned-up API.

..

>  static void pcim_iomap_release(struct device *gendev, void *res)
>  {
> -	struct pci_dev *dev = to_pci_dev(gendev);
> -	struct pcim_iomap_devres *this = res;
> -	int i;
> -
> -	for (i = 0; i < PCIM_IOMAP_MAX; i++)
> -		if (this->table[i])
> -			pci_iounmap(dev, this->table[i]);
> +	/*
> +	 * Do nothing. This is legacy code.
> +	 *
> +	 * Cleanup of the mappings is now done directly through the callbacks
> +	 * registered when creating them.
> +	 */
>  }

How many users we have? Can't we simply kill it for good?

..

> + * This function is DEPRECATED. Do not use it in new code.

We have __deprecated IIRC, can it be used?

..

> +	if (pcim_add_mapping_to_legacy_table(pdev, mapping, bar) != 0)

Redundant ' != 0' part.

> +		goto err_table;

..

> +static inline bool mask_contains_bar(int mask, int bar)
> +{
> +	return mask & (1 << bar);

BIT() ?

> +}

But I believe this function is not needed (see below).

..

>  /**
> - * pcim_iomap_regions - Request and iomap PCI BARs
> + * pcim_iomap_regions - Request and iomap PCI BARs (DEPRECATED)
>   * @pdev: PCI device to map IO resources for
>   * @mask: Mask of BARs to request and iomap
>   * @name: Name associated with the requests
>   *
> + * Returns 0 on success, negative error code on failure.

Validate the kernel-doc.

>   * Request and iomap regions specified by @mask.
> + *
> + * This function is DEPRECATED. Don't use it in new code.
> + * Use pcim_iomap_region() instead.
>   */

..

> +	for (bar = 0; bar < DEVICE_COUNT_RESOURCE; bar++) {
> +		if (!mask_contains_bar(mask, bar))
> +			continue;

NIH for_each_set_bit().

..

> +		ret = pcim_add_mapping_to_legacy_table(pdev, mapping, bar);
> +		if (ret != 0)

		if (ret)

> +			goto err;
> +	}

..

> + err:

Better to name it like

err_unmap_and_remove:

> +	while (--bar >= 0) {

	while (bar--)

is easier to read.

> +		pcim_iounmap_region(pdev, bar);
> +		pcim_remove_bar_from_legacy_table(pdev, bar);
> +	}

..

> +/**
> + * pcim_request_all_regions - Request all regions
> + * @pdev: PCI device to map IO resources for
> + * @name: name associated with the request
> + *
> + * Requested regions will automatically be released at driver detach. If desired,
> + * release individual regions with pcim_release_region() or all of them at once
> + * with pcim_release_all_regions().
> + */

Validate kernel-doc.

..

> +		ret = pcim_request_region(pdev, bar, name);
> +		if (ret != 0)

		if (ret)

> +			goto err;


..

> +	short bar;

Why signed?

> +	int ret = -1;

Oy vei!

..

> +	ret = pcim_request_all_regions(pdev, name);
> +	if (ret != 0)

Here and in plenty other places

	if (ret)

> +		return ret;

> +	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> +		if (!mask_contains_bar(mask, bar))
> +			continue;

NIH for_each_set_bit()

> +		if (!pcim_iomap(pdev, bar, 0))
> +			goto err;
> +	}

..

> +	if (!legacy_iomap_table)

What's wrong with positive check?

> +		ret = -ENOMEM;
> +	else
> +		ret = -EINVAL;

Can be even one liner


What's wrong with positive check?

		ret = legacy_iomap_table ? -EINVAL : -ENOMEM;

..

> +	while (--bar >= 0)

	while (bar--)

> +		pcim_iounmap(pdev, legacy_iomap_table[bar]);

..

> +	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> +		if (!mask_contains_bar(mask, bar))
>  			continue;

NIH for_each_set_bit()

> -		pcim_iounmap(pdev, iomap[i]);
> -		pci_release_region(pdev, i);
> +		pcim_iounmap_region(pdev, bar);
> +		pcim_remove_bar_from_legacy_table(pdev, bar);
>  	}

-- 
With Best Regards,
Andy Shevchenko