From: Mauro Carvalho Chehab <mchehab@redhat.com> To: <balbi@ti.com> Cc: Borislav Petkov <bp@alien8.de>, Greg KH <gregkh@linuxfoundation.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Steven Rostedt <rostedt@goodmis.org>, Frederic Weisbecker <fweisbec@gmail.com>, Ingo Molnar <mingo@redhat.com>, <JBottomley@parallels.com>, <linux-scsi@vger.kernel.org>, <davem@davemloft.net>, <netdev@vger.kernel.org>, Doug Thompson <dougthompson@xmission.com>, <linux-edac@vger.kernel.org>, <rjw@sisk.pl>, <linux-pm@vger.kernel.org> Subject: Re: SYSFS "errors" Date: Tue, 19 Feb 2013 08:11:49 -0300 [thread overview] Message-ID: <20130219081149.46972f56@redhat.com> (raw) In-Reply-To: <20130219101121.GJ23197@arwen.pp.htv.fi> [-- Attachment #1: Type: text/plain, Size: 3716 bytes --] Em Tue, 19 Feb 2013 12:11:21 +0200 Felipe Balbi <balbi@ti.com> escreveu: > Hi, > > On Tue, Feb 19, 2013 at 07:03:10AM -0300, Mauro Carvalho Chehab wrote: > > > But my gut feeling says to stay concervative and not touch this code - > > > we don't know what uses it and how much we would break by "fixing" it. > > > The current situation is not that big of a deal IMVHO and I'd be willing > > > to accept the small inconcistency versus possibly breaking userspace. > > > > I remember I saw some discussions about it in the past at bluesmoke ML, > > saying that -ENODEV is the expected behavior when this is not supported. > > > > Changing from -ENODEV to "N/A" will break anything that would be relying > > on the previous behavior. So, I think that such change will for sure break > > userspace. > > > > If we're willing to change it, not creating the "sdram_scrub_rate" sysfs > > node is less likely to affect userspace. > > yeah, I agree with this. Guess we shouldn't be creating files which > aren't supported by the underlying HW and having a read() return -ENODEV > is quite weird IMO since that's actually 'breaking' read() interface > although that's up to interpretations. The enclosed (untested) patch will likely do the trick. It needs to be tested with one of the drivers that support scrub setting (amd64_edac.c, cpc925_edac.c, e752x_edac.c, i5100_edac.c or i7core_edac.c). Regards, Mauro - [PATCH] edac: only create sdram_scrub_rate where supported Currently, sdram_scrub_rate sysfs node is created even if the device doesn't support get/set the scub rate. Change the logic to only create this device node when the operation is supported. Reported-by: Felipe Balbi <balbi@ti.com> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c index 9c58da6..937975b 100644 --- a/drivers/edac/edac_mc_sysfs.c +++ b/drivers/edac/edac_mc_sysfs.c @@ -7,7 +7,7 @@ * * Written Doug Thompson <norsk5@xmission.com> www.softwarebitmaker.com * - * (c) 2012 - Mauro Carvalho Chehab <mchehab@redhat.com> + * (c) 2012-2013 - Mauro Carvalho Chehab <mchehab@redhat.com> * The entire API were re-written, and ported to use struct device * */ @@ -681,9 +681,6 @@ static ssize_t mci_sdram_scrub_rate_store(struct device *dev, unsigned long bandwidth = 0; int new_bw = 0; - if (!mci->set_sdram_scrub_rate) - return -ENODEV; - if (strict_strtoul(data, 10, &bandwidth) < 0) return -EINVAL; @@ -707,9 +704,6 @@ static ssize_t mci_sdram_scrub_rate_show(struct device *dev, struct mem_ctl_info *mci = to_mci(dev); int bandwidth = 0; - if (!mci->get_sdram_scrub_rate) - return -ENODEV; - bandwidth = mci->get_sdram_scrub_rate(mci); if (bandwidth < 0) { edac_printk(KERN_DEBUG, EDAC_MC, "Error reading scrub rate\n"); @@ -882,7 +876,6 @@ static struct attribute *mci_attrs[] = { &dev_attr_ce_noinfo_count.attr, &dev_attr_ue_count.attr, &dev_attr_ce_count.attr, - &dev_attr_sdram_scrub_rate.attr, &dev_attr_max_location.attr, NULL }; @@ -1017,6 +1010,14 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci) return err; } + if (mci->set_sdram_scrub_rate && mci->get_sdram_scrub_rate) { + err = device_create_file(&mci->dev, + &dev_attr_sdram_scrub_rate); + if (err) { + edac_dbg(1, "failure: create sdram_scrub_rate\n"); + goto fail2; + } + } /* * Create the dimm/rank devices */ @@ -1061,6 +1062,7 @@ fail: continue; device_unregister(&dimm->dev); } +fail2: device_unregister(&mci->dev); bus_unregister(&mci->bus); kfree(mci->bus.name); [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 836 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: Mauro Carvalho Chehab <mchehab@redhat.com> To: balbi@ti.com Cc: Borislav Petkov <bp@alien8.de>, Greg KH <gregkh@linuxfoundation.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Steven Rostedt <rostedt@goodmis.org>, Frederic Weisbecker <fweisbec@gmail.com>, Ingo Molnar <mingo@redhat.com>, JBottomley@parallels.com, linux-scsi@vger.kernel.org, davem@davemloft.net, netdev@vger.kernel.org, Doug Thompson <dougthompson@xmission.com>, linux-edac@vger.kernel.org, rjw@sisk.pl, linux-pm@vger.kernel.org Subject: Re: SYSFS "errors" Date: Tue, 19 Feb 2013 08:11:49 -0300 [thread overview] Message-ID: <20130219081149.46972f56@redhat.com> (raw) In-Reply-To: <20130219101121.GJ23197@arwen.pp.htv.fi> [-- Attachment #1: Type: text/plain, Size: 3716 bytes --] Em Tue, 19 Feb 2013 12:11:21 +0200 Felipe Balbi <balbi@ti.com> escreveu: > Hi, > > On Tue, Feb 19, 2013 at 07:03:10AM -0300, Mauro Carvalho Chehab wrote: > > > But my gut feeling says to stay concervative and not touch this code - > > > we don't know what uses it and how much we would break by "fixing" it. > > > The current situation is not that big of a deal IMVHO and I'd be willing > > > to accept the small inconcistency versus possibly breaking userspace. > > > > I remember I saw some discussions about it in the past at bluesmoke ML, > > saying that -ENODEV is the expected behavior when this is not supported. > > > > Changing from -ENODEV to "N/A" will break anything that would be relying > > on the previous behavior. So, I think that such change will for sure break > > userspace. > > > > If we're willing to change it, not creating the "sdram_scrub_rate" sysfs > > node is less likely to affect userspace. > > yeah, I agree with this. Guess we shouldn't be creating files which > aren't supported by the underlying HW and having a read() return -ENODEV > is quite weird IMO since that's actually 'breaking' read() interface > although that's up to interpretations. The enclosed (untested) patch will likely do the trick. It needs to be tested with one of the drivers that support scrub setting (amd64_edac.c, cpc925_edac.c, e752x_edac.c, i5100_edac.c or i7core_edac.c). Regards, Mauro - [PATCH] edac: only create sdram_scrub_rate where supported Currently, sdram_scrub_rate sysfs node is created even if the device doesn't support get/set the scub rate. Change the logic to only create this device node when the operation is supported. Reported-by: Felipe Balbi <balbi@ti.com> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c index 9c58da6..937975b 100644 --- a/drivers/edac/edac_mc_sysfs.c +++ b/drivers/edac/edac_mc_sysfs.c @@ -7,7 +7,7 @@ * * Written Doug Thompson <norsk5@xmission.com> www.softwarebitmaker.com * - * (c) 2012 - Mauro Carvalho Chehab <mchehab@redhat.com> + * (c) 2012-2013 - Mauro Carvalho Chehab <mchehab@redhat.com> * The entire API were re-written, and ported to use struct device * */ @@ -681,9 +681,6 @@ static ssize_t mci_sdram_scrub_rate_store(struct device *dev, unsigned long bandwidth = 0; int new_bw = 0; - if (!mci->set_sdram_scrub_rate) - return -ENODEV; - if (strict_strtoul(data, 10, &bandwidth) < 0) return -EINVAL; @@ -707,9 +704,6 @@ static ssize_t mci_sdram_scrub_rate_show(struct device *dev, struct mem_ctl_info *mci = to_mci(dev); int bandwidth = 0; - if (!mci->get_sdram_scrub_rate) - return -ENODEV; - bandwidth = mci->get_sdram_scrub_rate(mci); if (bandwidth < 0) { edac_printk(KERN_DEBUG, EDAC_MC, "Error reading scrub rate\n"); @@ -882,7 +876,6 @@ static struct attribute *mci_attrs[] = { &dev_attr_ce_noinfo_count.attr, &dev_attr_ue_count.attr, &dev_attr_ce_count.attr, - &dev_attr_sdram_scrub_rate.attr, &dev_attr_max_location.attr, NULL }; @@ -1017,6 +1010,14 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci) return err; } + if (mci->set_sdram_scrub_rate && mci->get_sdram_scrub_rate) { + err = device_create_file(&mci->dev, + &dev_attr_sdram_scrub_rate); + if (err) { + edac_dbg(1, "failure: create sdram_scrub_rate\n"); + goto fail2; + } + } /* * Create the dimm/rank devices */ @@ -1061,6 +1062,7 @@ fail: continue; device_unregister(&dimm->dev); } +fail2: device_unregister(&mci->dev); bus_unregister(&mci->bus); kfree(mci->bus.name); [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 836 bytes --]
next prev parent reply other threads:[~2013-02-19 11:12 UTC|newest] Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top 2013-02-18 15:33 SYSFS "errors" Felipe Balbi 2013-02-18 15:50 ` Greg KH 2013-02-18 15:52 ` Felipe Balbi 2013-02-18 17:13 ` Greg KH 2013-02-18 17:27 ` Felipe Balbi 2013-02-18 17:45 ` Borislav Petkov 2013-02-18 18:47 ` Felipe Balbi 2013-02-18 19:40 ` Borislav Petkov 2013-02-18 20:04 ` Felipe Balbi 2013-02-18 17:49 ` Greg KH 2013-02-18 18:46 ` Felipe Balbi 2013-02-18 18:46 ` Felipe Balbi 2013-02-18 19:46 ` Mauro Carvalho Chehab 2013-02-18 19:46 ` Mauro Carvalho Chehab 2013-02-18 20:05 ` Felipe Balbi 2013-02-18 20:05 ` Felipe Balbi 2013-02-18 21:47 ` Mauro Carvalho Chehab 2013-02-18 21:47 ` Mauro Carvalho Chehab 2013-02-18 21:54 ` Greg KH 2013-02-18 22:13 ` Borislav Petkov 2013-02-18 22:26 ` Greg KH 2013-02-18 22:44 ` Borislav Petkov 2013-02-19 10:03 ` Mauro Carvalho Chehab 2013-02-19 10:11 ` Felipe Balbi 2013-02-19 10:11 ` Felipe Balbi 2013-02-19 11:11 ` Mauro Carvalho Chehab [this message] 2013-02-19 11:11 ` Mauro Carvalho Chehab 2013-02-19 11:43 ` Borislav Petkov 2013-02-19 12:16 ` Mauro Carvalho Chehab 2013-02-19 12:35 ` Borislav Petkov 2013-02-19 12:46 ` Mauro Carvalho Chehab 2013-02-19 13:06 ` Borislav Petkov 2013-02-19 13:15 ` Felipe Balbi 2013-02-19 13:15 ` Felipe Balbi 2013-02-19 13:28 ` Borislav Petkov 2013-02-19 13:38 ` Felipe Balbi 2013-02-19 13:38 ` Felipe Balbi 2013-02-19 13:50 ` Mauro Carvalho Chehab 2013-02-19 13:50 ` Mauro Carvalho Chehab 2013-02-19 13:55 ` Borislav Petkov 2013-02-19 13:50 ` Borislav Petkov 2013-02-19 13:58 ` Hannes Reinecke 2013-02-19 14:10 ` Borislav Petkov 2013-02-19 14:14 ` Mauro Carvalho Chehab 2013-02-19 14:19 ` Felipe Balbi 2013-02-19 14:19 ` Felipe Balbi 2013-02-19 14:35 ` Mauro Carvalho Chehab 2013-02-19 14:50 ` Borislav Petkov 2013-02-19 14:53 ` Felipe Balbi 2013-02-19 14:53 ` Felipe Balbi 2013-02-19 13:42 ` Mauro Carvalho Chehab 2013-02-18 21:48 ` Alan Stern 2013-02-18 21:48 ` Alan Stern 2013-02-18 21:57 ` Felipe Balbi 2013-02-18 21:57 ` Felipe Balbi 2013-02-19 7:41 ` Alexander Stein 2013-02-19 10:12 ` Felipe Balbi
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20130219081149.46972f56@redhat.com \ --to=mchehab@redhat.com \ --cc=JBottomley@parallels.com \ --cc=balbi@ti.com \ --cc=bp@alien8.de \ --cc=davem@davemloft.net \ --cc=dougthompson@xmission.com \ --cc=fweisbec@gmail.com \ --cc=gregkh@linuxfoundation.org \ --cc=linux-edac@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=linux-scsi@vger.kernel.org \ --cc=mingo@redhat.com \ --cc=netdev@vger.kernel.org \ --cc=rjw@sisk.pl \ --cc=rostedt@goodmis.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.