linux-kernel - Re: [PATCH v11 4/8] cxl/pci: Create PCI DOE mailbox's for memory devices

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: <YrIM+GnP+g0HbwaK@iweiny-desk3>
Date:   Tue, 21 Jun 2022 11:24:56 -0700
From:   Ira Weiny <ira.weiny@...el.com>
To:     Davidlohr Bueso <dave@...olabs.net>
CC:     Dan Williams <dan.j.williams@...el.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        "Ben Widawsky" <bwidawsk@...nel.org>,
        Alison Schofield <alison.schofield@...el.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        "Dave Jiang" <dave.jiang@...el.com>,
        <linux-kernel@...r.kernel.org>, <linux-cxl@...r.kernel.org>,
        <linux-pci@...r.kernel.org>, <a.manzanares@...sung.com>
Subject: Re: [PATCH v11 4/8] cxl/pci: Create PCI DOE mailbox's for memory
 devices

On Fri, Jun 17, 2022 at 01:40:46PM -0700, Davidlohr Bueso wrote:
> On Fri, 10 Jun 2022, ira.weiny@...el.com wrote:
> > +++ b/drivers/cxl/cxlmem.h
> > @@ -191,6 +191,8 @@ struct cxl_endpoint_dvsec_info {
> >  * @component_reg_phys: register base of component registers
> >  * @info: Cached DVSEC information about the device.
> >  * @serial: PCIe Device Serial Number
> 
> Missing doc:
> 
> @doe_use_irq: Use interrupt vectors for DOEs over polling.
> 
> However introducing such flags is not pretty, and this is only used by
> devm_cxl_pci_create_doe(). Do we really need it? See below.

Yes Dan had the same feedback to get rid of the member.

[snip]

> > +
> > static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > {
> > 	struct cxl_register_map map;
> > @@ -434,6 +545,9 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > 
> > 	cxlds->component_reg_phys = cxl_regmap_to_base(pdev, &map);
> > 
> > +	cxl_alloc_irq_vectors(cxlds);
> > +	devm_cxl_pci_create_doe(cxlds);
> 
> Should cxl_alloc_irq_vectors() just be called directly from devm_cxl_pci_create_doe()
> instead?

I was anticipating having to merge cxl_alloc_irq_vectors() with other code
which is supporting irq's for other things later on.  So I kept the 2 separate.
This is also why the use irq flag was in the device state.  The irq vector call
could have specified to use other types of irq's.  But those can be passed
directly as Dan suggested.

> Also if devm_cxl_pci_create_doe() fails (say ENOMEM), why do we
> bother continuing the cxl_pci probing?

Because the DOE is only required for CDAT data which is optional at this point.

Thanks for the suggested diff below.  But I'm going to go with Dan's suggestion
to use a flag which is set and passed between the functions.

Thanks for the review!
Ira

> 
> Thanks,
> Davidlohr
> 
> ------
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index ce5b00f3ebcb..44098c785a8b 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -230,7 +230,6 @@ struct cxl_dev_state {
> 	resource_size_t component_reg_phys;
> 	u64 serial;
> 
> -	bool doe_use_irq;
> 	struct pci_doe_mb **doe_mbs;
> 	int num_mbs;
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 72c7b535f5df..47c3741f7768 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -403,7 +403,7 @@ static void cxl_doe_destroy_mb(void *ds)
> 	}
>  }
> 
> -static void cxl_alloc_irq_vectors(struct cxl_dev_state *cxlds)
> +static int cxl_alloc_irq_vectors(struct cxl_dev_state *cxlds)
>  {
> 	struct device *dev = cxlds->dev;
> 	struct pci_dev *pdev = to_pci_dev(dev);
> @@ -421,9 +421,7 @@ static void cxl_alloc_irq_vectors(struct cxl_dev_state *cxlds)
> 	}
> 
> 	if (!max_irqs)
> -		return;
> -
> -	cxlds->doe_use_irq = false;
> +		return -ENOMEM;
> 
> 	/*
> 	 * Allocate enough vectors for the DOE's
> @@ -435,14 +433,10 @@ static void cxl_alloc_irq_vectors(struct cxl_dev_state *cxlds)
> 		/* Some got allocated; clean them up */
> 		if (rc > 0)
> 			cxl_pci_free_irq_vectors(pdev);
> -		return;
> +		return -ENOMEM;
> 	}
> 
> -	rc = devm_add_action_or_reset(dev, cxl_pci_free_irq_vectors, pdev);
> -	if (rc)
> -		return;
> -
> -	cxlds->doe_use_irq = true;
> +	return devm_add_action_or_reset(dev, cxl_pci_free_irq_vectors, pdev);
>  }
> 
>  /**
> @@ -457,6 +451,10 @@ static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
> 	u16 off = 0;
> 	int num_mbs = 0;
> 	int rc;
> +	bool doe_use_irq = false;
> +
> +	if (cxl_alloc_irq_vectors(cxlds))
> +		doe_use_irq = true;
> 
> 	pci_doe_for_each_off(pdev, off)
> 		num_mbs++;
> @@ -475,7 +473,7 @@ static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
> 		struct pci_doe_mb *doe_mb;
> 		int irq = -1;
> 
> -		if (cxlds->doe_use_irq)
> +		if (doe_use_irq)
> 			irq = pci_doe_get_irq_num(pdev, off);
> 
> 		doe_mb = pci_doe_create_mb(pdev, off, irq);
> @@ -545,7 +543,6 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> 
> 	cxlds->component_reg_phys = cxl_regmap_to_base(pdev, &map);
> 
> -	cxl_alloc_irq_vectors(cxlds);
> 	devm_cxl_pci_create_doe(cxlds);
> 
> 	rc = cxl_pci_setup_mailbox(cxlds);