linux-kernel - Re: [PATCH 12/21] ata: libahci: Discard redundant force_port

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 24 Mar 2022 11:05:58 +0900
From:   Damien Le Moal <damien.lemoal@...nsource.wdc.com>
To:     Serge Semin <Sergey.Semin@...kalelectronics.ru>,
        Hans de Goede <hdegoede@...hat.com>,
        Jens Axboe <axboe@...nel.dk>
Cc:     Serge Semin <fancer.lancer@...il.com>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Pavel Parkhomenko <Pavel.Parkhomenko@...kalelectronics.ru>,
        Rob Herring <robh+dt@...nel.org>, linux-ide@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 12/21] ata: libahci: Discard redundant force_port_map
 parameter

On 3/24/22 09:16, Serge Semin wrote:
> Currently there are four port-map-related fields declared in the
> ahci_host_priv structure and used to setup the HBA ports mapping. First
> the ports-mapping is read from the PI register and immediately stored in
> the saved_port_map field. If forced_port_map is initialized with non-zero
> value then its value will have greater priority over the value read from
> PI, thus it will override the saved_port_map field. That value will be then
> masked by a non-zero mask_port_map field and after some sanity checks it
> will be stored in the ahci_host_priv.port_map field as a final port
> mapping.
> 
> As you can see the logic is a bit too complicated for such a simple task.
> We can freely get rid from at least one of the fields with no change to
> the implemented semantic. The force_port_map field can be replaced with
> taking non-zero saved_port_map value into account. So if saved_port_map is
> pre-initialized by the glue-driver/platform-specific code then it will

glue-driver == LLDD (low level device driver), for the entire series please.

> have greater priority over the value read from PI register and will be
> used as actual HBA ports mapping later on. Thus the ports map forcing task
> will be just transferred from the force_port_map to saved_port_map field.
> 
> This modification will perfectly fit into the feature of having OF-based
> initialization of the HW-init HBA CSR fields we are about to introduce in
> the next commit.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@...kalelectronics.ru>
> ---
>  drivers/ata/ahci.c             |  2 +-
>  drivers/ata/ahci.h             |  1 -
>  drivers/ata/libahci.c          | 10 ++++++----
>  drivers/ata/libahci_platform.c |  2 +-
>  4 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index ab5811ef5a53..8ce0d166cc8d 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -654,7 +654,7 @@ static void ahci_pci_save_initial_config(struct pci_dev *pdev,
>  {
>  	if (pdev->vendor == PCI_VENDOR_ID_JMICRON && pdev->device == 0x2361) {
>  		dev_info(&pdev->dev, "JMB361 has only one port\n");
> -		hpriv->force_port_map = 1;
> +		hpriv->saved_port_map = 1;
>  	}
>  
>  	/*
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index 04690b4168a3..519d148ecaea 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -329,7 +329,6 @@ struct ahci_port_priv {
>  struct ahci_host_priv {
>  	/* Input fields */
>  	unsigned int		flags;		/* AHCI_HFLAG_* */
> -	u32			force_port_map;	/* force port map */
>  	u32			mask_port_map;	/* mask out particular bits */
>  
>  	void __iomem *		mmio;		/* bus-independent mem map */
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 0ed484e04fd6..011175e82174 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -453,7 +453,6 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
>  	 * reset.  Values without are used for driver operation.
>  	 */
>  	hpriv->saved_cap = cap = readl(mmio + HOST_CAP);
> -	hpriv->saved_port_map = port_map = readl(mmio + HOST_PORTS_IMPL);
>  
>  	/* CAP2 register is only defined for AHCI 1.2 and later */
>  	vers = readl(mmio + HOST_VERSION);
> @@ -517,10 +516,13 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
>  		cap &= ~HOST_CAP_SXS;
>  	}
>  
> -	if (hpriv->force_port_map && port_map != hpriv->force_port_map) {
> +	/* Override the HBA ports mapping if the platform needs it */
> +	port_map = readl(mmio + HOST_PORTS_IMPL);
> +	if (hpriv->saved_port_map && port_map != hpriv->saved_port_map) {
>  		dev_info(dev, "forcing port_map 0x%x -> 0x%x\n",
> -			 port_map, hpriv->force_port_map);
> -		port_map = hpriv->force_port_map;
> +			 port_map, hpriv->saved_port_map);
> +		port_map = hpriv->saved_port_map;
> +	} else {
>  		hpriv->saved_port_map = port_map;
>  	}
>  
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index febad33aa43c..5cbc2c42164d 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -539,7 +539,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>  	}
>  
>  	of_property_read_u32(dev->of_node,
> -			     "ports-implemented", &hpriv->force_port_map);
> +			     "ports-implemented", &hpriv->saved_port_map);
>  
>  	if (child_nodes) {
>  		for_each_child_of_node(dev->of_node, child) {


-- 
Damien Le Moal
Western Digital Research