linux-kernel - Re: [PATCH v2 1/2] PCI: altera: refactor driver for supporting new platform

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

Message-ID: <20230906170812.GA230066@bhelgaas>
Date:   Wed, 6 Sep 2023 12:08:12 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     sharath.kumar.d.m@...el.com
Cc:     lpieralisi@...nel.org, kw@...ux.com, robh@...nel.org,
        bhelgaas@...gle.com, linux-pci@...r.kernel.org,
        dinguyen@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] PCI: altera: refactor driver for supporting new
 platform

Capitalize subject line to match history:

  PCI: altera: Refactor driver for supporting new platform

On Wed, Sep 06, 2023 at 04:39:17PM +0530, sharath.kumar.d.m@...el.com wrote:
> From: D M Sharath Kumar <sharath.kumar.d.m@...el.com>

Needs a commit log.  It's OK to repeat the subject line.  Also helpful
if you can hint about why it needs to be refactored.  In this case, it
looks like you'll need to have a different ISR for Agilex, and also
something different about root bus vs downstream config accesses.

> @@ -100,10 +101,15 @@ struct altera_pcie_ops {
>  	void (*tlp_write_pkt)(struct altera_pcie *pcie, u32 *headers,
>  			      u32 data, bool align);
>  	bool (*get_link_status)(struct altera_pcie *pcie);
> -	int (*rp_read_cfg)(struct altera_pcie *pcie, int where,
> -			   int size, u32 *value);
> +	int (*rp_read_cfg)(struct altera_pcie *pcie, u8 busno,
> +			unsigned int devfn, int where, int size, u32 *value);
>  	int (*rp_write_cfg)(struct altera_pcie *pcie, u8 busno,
> -			    int where, int size, u32 value);
> +			unsigned int devfn, int where, int size, u32 value);
> +	int (*ep_read_cfg)(struct altera_pcie *pcie, u8 busno,
> +			unsigned int devfn, int where, int size, u32 *value);
> +	int (*ep_write_cfg)(struct altera_pcie *pcie, u8 busno,
> +			unsigned int devfn, int where, int size, u32 value);

"ep_read_cfg" isn't the ideal name because it suggests "endpoint", but
it may be either an endpoint or a switch upstream port.  The rockchip
driver uses "other", which isn't super descriptive either but might be
better.

> +	void (*rp_isr)(struct irq_desc *desc);
>  };

> +static int _altera_pcie_cfg_read(struct altera_pcie *pcie, u8 busno,
> +				 unsigned int devfn, int where, int size,
> +				 u32 *value)
> +{
> +	if (busno == pcie->root_bus_nr && pcie->pcie_data->ops->rp_read_cfg)
> +		return pcie->pcie_data->ops->rp_read_cfg(pcie, busno, devfn,
> +							where, size, value);

Several other drivers use pci_is_root_bus() instead of keeping track
of root_bus_nr and watching for changes to it.  Maybe simpler and more
reliable?  That would be best as a separate patch all by itself if you
go that direction.

> +
> +	if (pcie->pcie_data->ops->ep_read_cfg)
> +		return pcie->pcie_data->ops->ep_read_cfg(pcie, busno, devfn,
> +							where, size, value);
> +	return PCIBIOS_FUNC_NOT_SUPPORTED;
> +}