Linux-EDAC Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] EDAC/ie31200: fallback if host bridge device is already initialized
@ 2020-07-16 18:25 Jason Baron
  2020-07-16 18:52 ` Luck, Tony
  2020-08-13 13:44 ` Aristeu Rozanski
  0 siblings, 2 replies; 10+ messages in thread
From: Jason Baron @ 2020-07-16 18:25 UTC (permalink / raw)
  To: bp; +Cc: linux-kernel, Mauro Carvalho Chehab, Tony Luck, linux-edac

The Intel uncore driver may claim some of the pci ids from ie31200 which
means that the ie31200 edac driver will not initialize them as part of
pci_register_driver().

Let's add a fallback for this case to 'pci_get_device()' to get a
reference on the device such that it can still be configured. This is
similar in approach to other edac drivers.

Signed-off-by: Jason Baron <jbaron@akamai.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: linux-edac <linux-edac@vger.kernel.org>
---
 drivers/edac/ie31200_edac.c | 50 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 3 deletions(-)

diff --git a/drivers/edac/ie31200_edac.c b/drivers/edac/ie31200_edac.c
index d68346a..ebe5099 100644
--- a/drivers/edac/ie31200_edac.c
+++ b/drivers/edac/ie31200_edac.c
@@ -170,6 +170,8 @@
 	(n << (28 + (2 * skl) - PAGE_SHIFT))
 
 static int nr_channels;
+static struct pci_dev *mci_pdev;
+static int ie31200_registered = 1;
 
 struct ie31200_priv {
 	void __iomem *window;
@@ -538,12 +540,16 @@ static int ie31200_probe1(struct pci_dev *pdev, int dev_idx)
 static int ie31200_init_one(struct pci_dev *pdev,
 			    const struct pci_device_id *ent)
 {
-	edac_dbg(0, "MC:\n");
+	int rc;
 
+	edac_dbg(0, "MC:\n");
 	if (pci_enable_device(pdev) < 0)
 		return -EIO;
+	rc = ie31200_probe1(pdev, ent->driver_data);
+	if (rc == 0 && !mci_pdev)
+		mci_pdev = pci_dev_get(pdev);
 
-	return ie31200_probe1(pdev, ent->driver_data);
+	return rc;
 }
 
 static void ie31200_remove_one(struct pci_dev *pdev)
@@ -552,6 +558,8 @@ static void ie31200_remove_one(struct pci_dev *pdev)
 	struct ie31200_priv *priv;
 
 	edac_dbg(0, "\n");
+	pci_dev_put(mci_pdev);
+	mci_pdev = NULL;
 	mci = edac_mc_del_mc(&pdev->dev);
 	if (!mci)
 		return;
@@ -593,17 +601,53 @@ static struct pci_driver ie31200_driver = {
 
 static int __init ie31200_init(void)
 {
+	int pci_rc, i;
+
 	edac_dbg(3, "MC:\n");
 	/* Ensure that the OPSTATE is set correctly for POLL or NMI */
 	opstate_init();
 
-	return pci_register_driver(&ie31200_driver);
+	pci_rc = pci_register_driver(&ie31200_driver);
+	if (pci_rc < 0)
+		goto fail0;
+
+	if (!mci_pdev) {
+		ie31200_registered = 0;
+		for (i = 0; ie31200_pci_tbl[i].vendor != 0; i++) {
+			mci_pdev = pci_get_device(ie31200_pci_tbl[i].vendor,
+						  ie31200_pci_tbl[i].device,
+						  NULL);
+			if (mci_pdev)
+				break;
+		}
+		if (!mci_pdev) {
+			edac_dbg(0, "ie31200 pci_get_device fail\n");
+			pci_rc = -ENODEV;
+			goto fail1;
+		}
+		pci_rc = ie31200_init_one(mci_pdev, &ie31200_pci_tbl[i]);
+		if (pci_rc < 0) {
+			edac_dbg(0, "ie31200 init fail\n");
+			pci_rc = -ENODEV;
+			goto fail1;
+		}
+	}
+	return 0;
+
+fail1:
+	pci_unregister_driver(&ie31200_driver);
+fail0:
+	pci_dev_put(mci_pdev);
+
+	return pci_rc;
 }
 
 static void __exit ie31200_exit(void)
 {
 	edac_dbg(3, "MC:\n");
 	pci_unregister_driver(&ie31200_driver);
+	if (!ie31200_registered)
+		ie31200_remove_one(mci_pdev);
 }
 
 module_init(ie31200_init);
-- 
2.7.4


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] EDAC/ie31200: fallback if host bridge device is already initialized
  2020-07-16 18:25 [PATCH] EDAC/ie31200: fallback if host bridge device is already initialized Jason Baron
@ 2020-07-16 18:52 ` Luck, Tony
  2020-07-16 20:33   ` Jason Baron
  2020-08-13 13:44 ` Aristeu Rozanski
  1 sibling, 1 reply; 10+ messages in thread
From: Luck, Tony @ 2020-07-16 18:52 UTC (permalink / raw)
  To: Jason Baron; +Cc: bp, linux-kernel, Mauro Carvalho Chehab, linux-edac

On Thu, Jul 16, 2020 at 02:25:11PM -0400, Jason Baron wrote:
> The Intel uncore driver may claim some of the pci ids from ie31200 which
> means that the ie31200 edac driver will not initialize them as part of
> pci_register_driver().
> 
> Let's add a fallback for this case to 'pci_get_device()' to get a
> reference on the device such that it can still be configured. This is
> similar in approach to other edac drivers.

What functionality is lost when this happens?

Does the user see some message in the console
log to let them know?

-Tony

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] EDAC/ie31200: fallback if host bridge device is already initialized
  2020-07-16 18:52 ` Luck, Tony
@ 2020-07-16 20:33   ` Jason Baron
  2020-08-06 21:35     ` Jason Baron
  0 siblings, 1 reply; 10+ messages in thread
From: Jason Baron @ 2020-07-16 20:33 UTC (permalink / raw)
  To: Luck, Tony; +Cc: bp, linux-kernel, Mauro Carvalho Chehab, linux-edac



On 7/16/20 2:52 PM, Luck, Tony wrote:
> On Thu, Jul 16, 2020 at 02:25:11PM -0400, Jason Baron wrote:
>> The Intel uncore driver may claim some of the pci ids from ie31200 which
>> means that the ie31200 edac driver will not initialize them as part of
>> pci_register_driver().
>>
>> Let's add a fallback for this case to 'pci_get_device()' to get a
>> reference on the device such that it can still be configured. This is
>> similar in approach to other edac drivers.
> 
> What functionality is lost when this happens?

There is no difference in functionality when the fallback occurs. It should
operate in the same it does when the uncore driver is not loaded.

> 
> Does the user see some message in the console
> log to let them know?

I don't think its needed as there should be no user-visible difference.

Thanks,

-Jason

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] EDAC/ie31200: fallback if host bridge device is already initialized
  2020-07-16 20:33   ` Jason Baron
@ 2020-08-06 21:35     ` Jason Baron
  2020-08-06 23:32       ` Luck, Tony
  0 siblings, 1 reply; 10+ messages in thread
From: Jason Baron @ 2020-08-06 21:35 UTC (permalink / raw)
  To: bp; +Cc: Luck, Tony, linux-kernel, Mauro Carvalho Chehab, linux-edac



On 7/16/20 4:33 PM, Jason Baron wrote:
> 
> 
> On 7/16/20 2:52 PM, Luck, Tony wrote:
>> On Thu, Jul 16, 2020 at 02:25:11PM -0400, Jason Baron wrote:
>>> The Intel uncore driver may claim some of the pci ids from ie31200 which
>>> means that the ie31200 edac driver will not initialize them as part of
>>> pci_register_driver().
>>>

Hi,

I just wondering if there is any feedback on this issue, without this
patch the ie31200 edac driver doesn't load properly on a number of boxes.

Thanks,

-Jason

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] EDAC/ie31200: fallback if host bridge device is already initialized
  2020-08-06 21:35     ` Jason Baron
@ 2020-08-06 23:32       ` Luck, Tony
  0 siblings, 0 replies; 10+ messages in thread
From: Luck, Tony @ 2020-08-06 23:32 UTC (permalink / raw)
  To: Jason Baron; +Cc: bp, linux-kernel, Mauro Carvalho Chehab, linux-edac

On Thu, Aug 06, 2020 at 05:35:49PM -0400, Jason Baron wrote:
> 
> 
> On 7/16/20 4:33 PM, Jason Baron wrote:
> > 
> > 
> > On 7/16/20 2:52 PM, Luck, Tony wrote:
> >> On Thu, Jul 16, 2020 at 02:25:11PM -0400, Jason Baron wrote:
> >>> The Intel uncore driver may claim some of the pci ids from ie31200 which
> >>> means that the ie31200 edac driver will not initialize them as part of
> >>> pci_register_driver().
> >>>
> 
> Hi,
> 
> I just wondering if there is any feedback on this issue, without this
> patch the ie31200 edac driver doesn't load properly on a number of boxes.

Applied it now.  I'll see if I can get it merged for v5.9 (since
you posted in plenty of time to make this merge window).

-Tony

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] EDAC/ie31200: fallback if host bridge device is already initialized
  2020-07-16 18:25 [PATCH] EDAC/ie31200: fallback if host bridge device is already initialized Jason Baron
  2020-07-16 18:52 ` Luck, Tony
@ 2020-08-13 13:44 ` Aristeu Rozanski
  2020-08-13 13:55   ` Boris Petkov
  1 sibling, 1 reply; 10+ messages in thread
From: Aristeu Rozanski @ 2020-08-13 13:44 UTC (permalink / raw)
  To: Jason Baron
  Cc: bp, linux-kernel, Mauro Carvalho Chehab, Tony Luck, linux-edac

On Thu, Jul 16, 2020 at 02:25:11PM -0400, Jason Baron wrote:
> The Intel uncore driver may claim some of the pci ids from ie31200 which
> means that the ie31200 edac driver will not initialize them as part of
> pci_register_driver().
> 
> Let's add a fallback for this case to 'pci_get_device()' to get a
> reference on the device such that it can still be configured. This is
> similar in approach to other edac drivers.
> 
> Signed-off-by: Jason Baron <jbaron@akamai.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: linux-edac <linux-edac@vger.kernel.org>
> ---
>  drivers/edac/ie31200_edac.c | 50 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 47 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/edac/ie31200_edac.c b/drivers/edac/ie31200_edac.c
> index d68346a..ebe5099 100644
> --- a/drivers/edac/ie31200_edac.c
> +++ b/drivers/edac/ie31200_edac.c
> @@ -170,6 +170,8 @@
>  	(n << (28 + (2 * skl) - PAGE_SHIFT))
>  
>  static int nr_channels;
> +static struct pci_dev *mci_pdev;
> +static int ie31200_registered = 1;
>  
>  struct ie31200_priv {
>  	void __iomem *window;
> @@ -538,12 +540,16 @@ static int ie31200_probe1(struct pci_dev *pdev, int dev_idx)
>  static int ie31200_init_one(struct pci_dev *pdev,
>  			    const struct pci_device_id *ent)
>  {
> -	edac_dbg(0, "MC:\n");
> +	int rc;
>  
> +	edac_dbg(0, "MC:\n");
>  	if (pci_enable_device(pdev) < 0)
>  		return -EIO;
> +	rc = ie31200_probe1(pdev, ent->driver_data);
> +	if (rc == 0 && !mci_pdev)
> +		mci_pdev = pci_dev_get(pdev);
>  
> -	return ie31200_probe1(pdev, ent->driver_data);
> +	return rc;
>  }
>  
>  static void ie31200_remove_one(struct pci_dev *pdev)
> @@ -552,6 +558,8 @@ static void ie31200_remove_one(struct pci_dev *pdev)
>  	struct ie31200_priv *priv;
>  
>  	edac_dbg(0, "\n");
> +	pci_dev_put(mci_pdev);
> +	mci_pdev = NULL;
>  	mci = edac_mc_del_mc(&pdev->dev);
>  	if (!mci)
>  		return;
> @@ -593,17 +601,53 @@ static struct pci_driver ie31200_driver = {
>  
>  static int __init ie31200_init(void)
>  {
> +	int pci_rc, i;
> +
>  	edac_dbg(3, "MC:\n");
>  	/* Ensure that the OPSTATE is set correctly for POLL or NMI */
>  	opstate_init();
>  
> -	return pci_register_driver(&ie31200_driver);
> +	pci_rc = pci_register_driver(&ie31200_driver);
> +	if (pci_rc < 0)
> +		goto fail0;
> +
> +	if (!mci_pdev) {
> +		ie31200_registered = 0;
> +		for (i = 0; ie31200_pci_tbl[i].vendor != 0; i++) {
> +			mci_pdev = pci_get_device(ie31200_pci_tbl[i].vendor,
> +						  ie31200_pci_tbl[i].device,
> +						  NULL);
> +			if (mci_pdev)
> +				break;
> +		}
> +		if (!mci_pdev) {
> +			edac_dbg(0, "ie31200 pci_get_device fail\n");
> +			pci_rc = -ENODEV;
> +			goto fail1;
> +		}
> +		pci_rc = ie31200_init_one(mci_pdev, &ie31200_pci_tbl[i]);
> +		if (pci_rc < 0) {
> +			edac_dbg(0, "ie31200 init fail\n");
> +			pci_rc = -ENODEV;
> +			goto fail1;
> +		}
> +	}
> +	return 0;
> +
> +fail1:
> +	pci_unregister_driver(&ie31200_driver);
> +fail0:
> +	pci_dev_put(mci_pdev);
> +
> +	return pci_rc;
>  }
>  
>  static void __exit ie31200_exit(void)
>  {
>  	edac_dbg(3, "MC:\n");
>  	pci_unregister_driver(&ie31200_driver);
> +	if (!ie31200_registered)
> +		ie31200_remove_one(mci_pdev);
>  }
>  
>  module_init(ie31200_init);

We tested this inside in machines having this issue and it works.
Patch looks good to me.

Acked-by: Aristeu Rozanski <aris@redhat.com>

-- 
Aristeu


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] EDAC/ie31200: fallback if host bridge device is already initialized
  2020-08-13 13:44 ` Aristeu Rozanski
@ 2020-08-13 13:55   ` Boris Petkov
  2020-08-13 14:17     ` Aristeu Rozanski
  0 siblings, 1 reply; 10+ messages in thread
From: Boris Petkov @ 2020-08-13 13:55 UTC (permalink / raw)
  To: Aristeu Rozanski, Jason Baron
  Cc: linux-kernel, Mauro Carvalho Chehab, Tony Luck, linux-edac

On August 13, 2020 4:44:06 PM GMT+03:00, Aristeu Rozanski <aris@redhat.com> wrote:
>We tested this inside in machines having this issue and it works.
>Patch looks good to me.
>
>Acked-by: Aristeu Rozanski <aris@redhat.com>

So Tested-by: you ?


-- 
Sent from a small device: formatting sux and brevity is inevitable.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] EDAC/ie31200: fallback if host bridge device is already initialized
  2020-08-13 13:55   ` Boris Petkov
@ 2020-08-13 14:17     ` Aristeu Rozanski
  2020-08-13 14:55       ` Boris Petkov
  0 siblings, 1 reply; 10+ messages in thread
From: Aristeu Rozanski @ 2020-08-13 14:17 UTC (permalink / raw)
  To: Boris Petkov
  Cc: Jason Baron, linux-kernel, Mauro Carvalho Chehab, Tony Luck, linux-edac

On Thu, Aug 13, 2020 at 04:55:50PM +0300, Boris Petkov wrote:
> On August 13, 2020 4:44:06 PM GMT+03:00, Aristeu Rozanski <aris@redhat.com> wrote:
> >We tested this inside in machines having this issue and it works.
> >Patch looks good to me.
> >
> >Acked-by: Aristeu Rozanski <aris@redhat.com>
> 
> So Tested-by: you ?

Not by me, "we" meant as in company.

Tested-by: Vishal Agrawal <vagrawal@redhat.com>

-- 
Aristeu


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] EDAC/ie31200: fallback if host bridge device is already initialized
  2020-08-13 14:17     ` Aristeu Rozanski
@ 2020-08-13 14:55       ` Boris Petkov
  2020-08-13 15:56         ` Luck, Tony
  0 siblings, 1 reply; 10+ messages in thread
From: Boris Petkov @ 2020-08-13 14:55 UTC (permalink / raw)
  To: Aristeu Rozanski
  Cc: Jason Baron, linux-kernel, Mauro Carvalho Chehab, Tony Luck, linux-edac

On August 13, 2020 5:17:10 PM GMT+03:00, Aristeu Rozanski <aris@redhat.com> wrote:
>> So Tested-by: you ?
>
>Not by me, "we" meant as in company.

"you" can also mean you as a company. ;-P

>Tested-by: Vishal Agrawal <vagrawal@redhat.com>

Thx.


-- 
Sent from a small device: formatting sux and brevity is inevitable.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] EDAC/ie31200: fallback if host bridge device is already initialized
  2020-08-13 14:55       ` Boris Petkov
@ 2020-08-13 15:56         ` Luck, Tony
  0 siblings, 0 replies; 10+ messages in thread
From: Luck, Tony @ 2020-08-13 15:56 UTC (permalink / raw)
  To: Boris Petkov, Aristeu Rozanski
  Cc: Jason Baron, linux-kernel, Mauro Carvalho Chehab, linux-edac

> >Tested-by: Vishal Agrawal <vagrawal@redhat.com>

Boris,

I applied this patch when Jason Baron sent a reminder last week. It is sitting in the
ie31200 topic branch of the RAS tree (and also in the next branch ... so has had a
couple of days in linux-next too).

That copy doesn't have these Acked-by and Tested-by tags, but is otherwise the
same.

I plan to send a second edac pull request to Linus today/romorrow to pick it up.

-Tony

 

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-16 18:25 [PATCH] EDAC/ie31200: fallback if host bridge device is already initialized Jason Baron
2020-07-16 18:52 ` Luck, Tony
2020-07-16 20:33   ` Jason Baron
2020-08-06 21:35     ` Jason Baron
2020-08-06 23:32       ` Luck, Tony
2020-08-13 13:44 ` Aristeu Rozanski
2020-08-13 13:55   ` Boris Petkov
2020-08-13 14:17     ` Aristeu Rozanski
2020-08-13 14:55       ` Boris Petkov
2020-08-13 15:56         ` Luck, Tony

Linux-EDAC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-edac/0 linux-edac/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-edac linux-edac/ https://lore.kernel.org/linux-edac \
		linux-edac@vger.kernel.org
	public-inbox-index linux-edac

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-edac


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git