* Re: SOCK_STREAM TCP: send() returns success even when other side responded with RST packet
From: Stephen Hemminger @ 2013-02-19 15:26 UTC (permalink / raw)
To: Tomas Hozza; +Cc: Eric Dumazet, netdev
In-Reply-To: <2084007858.1889873.1361286562542.JavaMail.root@redhat.com>
> stack did not yet delivered the data.
>
> I'm using TCP because it should guarantee that my data were delivered or
> let me know there was some problem. If this is not a bug, then it is at least
> confusing for TCP.
>
> > To make sure data is delivered, you need additional logic.
>
> To be honest I didn't find any way how to get notified there was a RST packet
> sent as a reply to my previously sent data.
TCP makes no end-to-end (application to application) guarantees.
Applications that need assurance need to do a request-reply at end of
transaction. You should get a good textbook like Stevens for more detail
^ permalink raw reply
* Last call iproute2 for 3.8
From: Stephen Hemminger @ 2013-02-19 15:21 UTC (permalink / raw)
To: netdev
I plan to finalize iproute2 for 3.8 today and release by end of week.
Will open for 3.9 related patches after that
^ permalink raw reply
* Re: SOCK_STREAM TCP: send() returns success even when other side responded with RST packet
From: Tomas Hozza @ 2013-02-19 15:09 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1361285472.19353.126.camel@edumazet-glaptop>
----- Original Message -----
> On Tue, 2013-02-19 at 02:02 -0500, Tomas Hozza wrote:
> > Hi.
> >
> > I was advised to forward my networking Kernel related Bug here
> > so it can get fixed much faster. Here is the Bug description
> > copied from Red Hat Bugzilla
> > (https://bugzilla.redhat.com/show_bug.cgi?id=912382):
> >
> > Description of problem:
> > I'm using SOCK_STREAM TCP sockets. Imagine server-client
> > application.
> > - Server will accept connection from client.
> > - Client sends some data to server -> server receives data.
> > - Client then half-closes the TCP connection using shutdown(fd,
> > SHUT_WR).
> > - Client waits for some data from the server.
> > - Server sends some data to client -> client receives data.
> > - Client exits and closes the socket.
> > - After some time server tries to send some data to the client.
> > - The first send() call returns success even the Client's response
> > is RST.
> >
> >
> > Version-Release number of selected component (if applicable):
> > 3.7.7-201.fc18.x86_64
> > 3.7.8-202.fc18.x86_64
> >
> >
> > How reproducible:
> > always
> >
> >
> > Steps to Reproduce:
> > 1. download attached client/server sources.
> > 2. compile server: # gcc tcp_server.c -o server
> > 3. compile client: # gcc tcp_client.c -o client
> > 4. run the server: # ./server
> > 5. run the client: # ./client
> >
> >
> > Actual results:
> > Server started...
> > Created SOCK_STREAM socket
> > Bound to localhost:6666
> > waiting for connection...
> > Client connected!
> > Waiting for some data from client
> > Received: "MSG from client"
> > Received EOF
> > Sending msg to client: "MSG from server"
> > sleep(5)
> > Sending 1 msg to client: "MSG from server"
> > try #1 send() result: (0) Success
> > Sending 2 msg to client: "MSG from server"
> > try #2 send() result: (32) Broken pipe
> > Server exiting...
> >
> >
> > Expected results:
> > Server should fail to send() data right after sleep().
> >
> > Server started...
> > Created SOCK_STREAM socket
> > Bound to localhost:6666
> > waiting for connection...
> > Client connected!
> > Waiting for some data from client
> > Received: "MSG from client"
> > Received EOF
> > Sending msg to client: "MSG from server"
> > sleep(5)
> > Sending 1 msg to client: "MSG from server"
> > try #1 send() result: (32) Broken pipe
> > Server exiting...
> >
> >
> > Additional info:
> > RST answer means that data were NOT delivered. Therefore the very
> > first send() call after server sleep() should fail!
> >
> >
> > Regards,
> >
> > Tomas Hozza
>
> Hi Tomas
>
> I read your bug report, but cant understand why you believe there is
> a bug.
>
> send() is not synchronous.
OK, my bad. In man page there is: "Successful completion of a call to
send() does not guarantee delivery of the message."
>
> It only queues bytes to be transmitted either right now, either later
> when its possible.
>
> So a RST message can be received _after_ send() has accepted the
> request and copy data from user land to kernel socket write queue, but TCP
> stack did not yet delivered the data.
I'm using TCP because it should guarantee that my data were delivered or
let me know there was some problem. If this is not a bug, then it is at least
confusing for TCP.
> To make sure data is delivered, you need additional logic.
To be honest I didn't find any way how to get notified there was a RST packet
sent as a reply to my previously sent data.
Thank you for your reply.
Regards,
Tomas Hozza
^ permalink raw reply
* Re: [PATCH v2 0/5] Generic PHY Framework
From: Felipe Balbi @ 2013-02-19 15:05 UTC (permalink / raw)
To: Arnd Bergmann
Cc: balbi, kishon, rob, tony, linux, eballetbo, javier, gregkh, akpm,
mchehab, cesarb, davem, santosh.shilimkar, broonie, swarren,
linux-doc, linux-kernel, linux-arm-kernel, linux-omap, linux-usb,
netdev
In-Reply-To: <201302191434.40495.arnd@arndb.de>
[-- Attachment #1: Type: text/plain, Size: 3177 bytes --]
Hi,
On Tue, Feb 19, 2013 at 02:34:40PM +0000, Arnd Bergmann wrote:
> On Tuesday 19 February 2013, Felipe Balbi wrote:
> > On Tue, Feb 19, 2013 at 12:33:54PM +0000, Arnd Bergmann wrote:
> > > > Currently drivers/phy and drivers/net/phy are independent and are not
> > > > related to each other. There are some fundamental differences on how
> > > > these frameworks work. IIUC, the *net* uses bus layer (MDIO bus) to
> > > > match a PHY device with a PHY driver and the Ethernet device uses the
> > > > bus layer to get the PHY.
> > > > The Generic PHY Framework however doesn't have any bus layer. The PHY
> > > > should be like any other Platform Devices and Drivers and the framework
> > > > will provide some APIs to register with the framework. And there are
> > > > other APIs which any controller can use to get the PHY (for e.g., in the
> > > > case of dt boot, it can use phandle to get a reference to the PHY).
> > >
> > > Hmm, I think the use of a bus_type for a PHY actually sounds quite
> > > appropriate. The actual PHY device would then be a child of the
> >
> > really ? I'm not so sure, the *bus* used by the PHY is ULPI, UTMI,
> > UTMI+, PIP3, I2C, etc... adding another 'fake' bus representation is a
> > bit overkill IMO.
> >
> > Imagine an I2C-controlled PHY driver like isp1301, that driver will have
> > to register i2c_driver and phy_driver, which looks weird to me. If the
> > only substitute for class is a bus, we can't drop classes just yet, I'm
> > afraid.
> >
> > Imagine a regulator bus, a pwm bus, an LED bus etc. They don't make
> > sense IMHO.
>
> It's a fine line, but I think a phy is something that resembles a device
> more than an LED does. When I read patch 1, I also noticed and commented
> on the fact that it does use a 'class'. Now, according to Greg, we should
> use 'bus_type' instead of 'class' in new code. I originally disagreed with
> that concept, but he's the boss here and it's good if he has a vision
> how things should be lined out.
>
> In practice, there is little difference between a 'bus_type' and a 'class',
> so just replace any instance of the former with the latter in your head
> when reading the code ;-)
it's not so simple :-) if we must use bus_type we need to introduce all
the device/driver matching mechanism which isn't necessary with a class.
> I understand that there is not a real common bus here, and the bus_type
> infrastructure would basically be used as a way to represent each PHY
> in sysfs and provide a way to enumerate and look them up inside of the
> kernel.
right, but maybe we need another mechanism. If, in the long run we must
use bus_type, then eventually pwm, led, regulators, etc will all be
converted to bus_type. It will look quite weird IMHO.
Greg, can you pitch your suggestion here ? It would be great to hear
your rationale behind dropping class infrastructure, couldn't find
anything through Google and since feature-removal-schedule.txt has been
removed (without adding it to feature-removal-schedule.txt, I must add
:-) I don't know what's the idea behind removing classes.
cheers
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: SYSFS "errors"
From: Felipe Balbi @ 2013-02-19 14:53 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Borislav Petkov, Felipe Balbi, Greg KH, Linux Kernel Mailing List,
Steven Rostedt, Frederic Weisbecker, Ingo Molnar, JBottomley,
linux-scsi, davem, netdev, Doug Thompson, linux-edac, rjw,
linux-pm
In-Reply-To: <20130219113521.510db290@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1343 bytes --]
On Tue, Feb 19, 2013 at 11:35:21AM -0300, Mauro Carvalho Chehab wrote:
> Em Tue, 19 Feb 2013 14:50:56 +0100
> Borislav Petkov <bp@alien8.de> escreveu:
>
> > On Tue, Feb 19, 2013 at 03:38:09PM +0200, Felipe Balbi wrote:
> > > because changing the permission will cause the same issue:
> >
> > Actually, I take that back. Mauro's patch will already create the file
> > anyway:
> >
> > if (mci->set_sdram_scrub_rate || mci->get_sdram_scrub_rate)
> >
> > Adjusting the permissions is simply the last missing piece to this patch
> > to make the interface to userspace 100% coherent.
>
> That's covers everything but Hannes arguments. I don't think that
> adding just one more device_create_file() on a driver that creates
> dozens (or hundreds) of sys nodes, depending on the number of DIMMS
> on the system would make it any worse.
>
> Regards,
> Mauro
>
> [PATCH EDAC] 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>
looks alright to me:
Reviewed-by: Felipe Balbi <balbi@ti.com>
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: SOCK_STREAM TCP: send() returns success even when other side responded with RST packet
From: Eric Dumazet @ 2013-02-19 14:51 UTC (permalink / raw)
To: Tomas Hozza; +Cc: netdev
In-Reply-To: <20690238.1777611.1361257332454.JavaMail.root@redhat.com>
On Tue, 2013-02-19 at 02:02 -0500, Tomas Hozza wrote:
> Hi.
>
> I was advised to forward my networking Kernel related Bug here
> so it can get fixed much faster. Here is the Bug description
> copied from Red Hat Bugzilla (https://bugzilla.redhat.com/show_bug.cgi?id=912382):
>
> Description of problem:
> I'm using SOCK_STREAM TCP sockets. Imagine server-client application.
> - Server will accept connection from client.
> - Client sends some data to server -> server receives data.
> - Client then half-closes the TCP connection using shutdown(fd, SHUT_WR).
> - Client waits for some data from the server.
> - Server sends some data to client -> client receives data.
> - Client exits and closes the socket.
> - After some time server tries to send some data to the client.
> - The first send() call returns success even the Client's response is RST.
>
>
> Version-Release number of selected component (if applicable):
> 3.7.7-201.fc18.x86_64
> 3.7.8-202.fc18.x86_64
>
>
> How reproducible:
> always
>
>
> Steps to Reproduce:
> 1. download attached client/server sources.
> 2. compile server: # gcc tcp_server.c -o server
> 3. compile client: # gcc tcp_client.c -o client
> 4. run the server: # ./server
> 5. run the client: # ./client
>
>
> Actual results:
> Server started...
> Created SOCK_STREAM socket
> Bound to localhost:6666
> waiting for connection...
> Client connected!
> Waiting for some data from client
> Received: "MSG from client"
> Received EOF
> Sending msg to client: "MSG from server"
> sleep(5)
> Sending 1 msg to client: "MSG from server"
> try #1 send() result: (0) Success
> Sending 2 msg to client: "MSG from server"
> try #2 send() result: (32) Broken pipe
> Server exiting...
>
>
> Expected results:
> Server should fail to send() data right after sleep().
>
> Server started...
> Created SOCK_STREAM socket
> Bound to localhost:6666
> waiting for connection...
> Client connected!
> Waiting for some data from client
> Received: "MSG from client"
> Received EOF
> Sending msg to client: "MSG from server"
> sleep(5)
> Sending 1 msg to client: "MSG from server"
> try #1 send() result: (32) Broken pipe
> Server exiting...
>
>
> Additional info:
> RST answer means that data were NOT delivered. Therefore the very
> first send() call after server sleep() should fail!
>
>
> Regards,
>
> Tomas Hozza
Hi Tomas
I read your bug report, but cant understand why you believe there is a
bug.
send() is not synchronous.
It only queues bytes to be transmitted either right now, either later
when its possible.
So a RST message can be received _after_ send() has accepted the request
and copy data from user land to kernel socket write queue, but TCP stack
did not yet delivered the data.
To make sure data is delivered, you need additional logic.
^ permalink raw reply
* Re: SYSFS "errors"
From: Borislav Petkov @ 2013-02-19 14:50 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Felipe Balbi, Greg KH, Linux Kernel Mailing List, Steven Rostedt,
Frederic Weisbecker, Ingo Molnar, JBottomley, linux-scsi, davem,
netdev, Doug Thompson, linux-edac, rjw, linux-pm
In-Reply-To: <20130219113521.510db290@redhat.com>
On Tue, Feb 19, 2013 at 11:35:21AM -0300, Mauro Carvalho Chehab wrote:
> That's covers everything but Hannes arguments. I don't think that
> adding just one more device_create_file() on a driver that creates
> dozens (or hundreds) of sys nodes, depending on the number of DIMMS
> on the system would make it any worse.
Right, those attributes are created once during EDAC core init and we
haven't seen any issues so far so let's not address this just yet but
make a mental note about the race condition possibility.
> [PATCH EDAC] 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>
Acked-by: Borislav Petkov <bp@suse.de>
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
^ permalink raw reply
* Re: SYSFS "errors"
From: Mauro Carvalho Chehab @ 2013-02-19 14:35 UTC (permalink / raw)
To: Borislav Petkov
Cc: Felipe Balbi, Greg KH, Linux Kernel Mailing List, Steven Rostedt,
Frederic Weisbecker, Ingo Molnar, JBottomley, linux-scsi, davem,
netdev, Doug Thompson, linux-edac, rjw, linux-pm
In-Reply-To: <20130219135056.GH26623@pd.tnic>
Em Tue, 19 Feb 2013 14:50:56 +0100
Borislav Petkov <bp@alien8.de> escreveu:
> On Tue, Feb 19, 2013 at 03:38:09PM +0200, Felipe Balbi wrote:
> > because changing the permission will cause the same issue:
>
> Actually, I take that back. Mauro's patch will already create the file
> anyway:
>
> if (mci->set_sdram_scrub_rate || mci->get_sdram_scrub_rate)
>
> Adjusting the permissions is simply the last missing piece to this patch
> to make the interface to userspace 100% coherent.
That's covers everything but Hannes arguments. I don't think that
adding just one more device_create_file() on a driver that creates
dozens (or hundreds) of sys nodes, depending on the number of DIMMS
on the system would make it any worse.
Regards,
Mauro
[PATCH EDAC] 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..4f4b613 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");
@@ -870,8 +864,7 @@ DEVICE_ATTR(ce_count, S_IRUGO, mci_ce_count_show, NULL);
DEVICE_ATTR(max_location, S_IRUGO, mci_max_location_show, NULL);
/* memory scrubber attribute file */
-DEVICE_ATTR(sdram_scrub_rate, S_IRUGO | S_IWUSR, mci_sdram_scrub_rate_show,
- mci_sdram_scrub_rate_store);
+DEVICE_ATTR(sdram_scrub_rate, 0, NULL, NULL);
static struct attribute *mci_attrs[] = {
&dev_attr_reset_counters.attr,
@@ -882,7 +875,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 +1009,22 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci)
return err;
}
+ if (mci->set_sdram_scrub_rate || mci->get_sdram_scrub_rate) {
+ if (mci->get_sdram_scrub_rate) {
+ dev_attr_sdram_scrub_rate.attr.mode |= S_IRUGO;
+ dev_attr_sdram_scrub_rate.show = &mci_sdram_scrub_rate_show;
+ }
+ if (mci->set_sdram_scrub_rate) {
+ dev_attr_sdram_scrub_rate.attr.mode |= S_IWUSR;
+ dev_attr_sdram_scrub_rate.store = &mci_sdram_scrub_rate_store;
+ }
+ 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 +1069,7 @@ fail:
continue;
device_unregister(&dimm->dev);
}
+fail2:
device_unregister(&mci->dev);
bus_unregister(&mci->bus);
kfree(mci->bus.name);
--
Cheers,
Mauro
^ permalink raw reply related
* Re: [PATCH v2 0/5] Generic PHY Framework
From: Arnd Bergmann @ 2013-02-19 14:34 UTC (permalink / raw)
To: balbi
Cc: kishon, rob, tony, linux, eballetbo, javier, gregkh, akpm,
mchehab, cesarb, davem, santosh.shilimkar, broonie, swarren,
linux-doc, linux-kernel, linux-arm-kernel, linux-omap, linux-usb,
netdev
In-Reply-To: <20130219131258.GV23197@arwen.pp.htv.fi>
On Tuesday 19 February 2013, Felipe Balbi wrote:
> On Tue, Feb 19, 2013 at 12:33:54PM +0000, Arnd Bergmann wrote:
> > > Currently drivers/phy and drivers/net/phy are independent and are not
> > > related to each other. There are some fundamental differences on how
> > > these frameworks work. IIUC, the *net* uses bus layer (MDIO bus) to
> > > match a PHY device with a PHY driver and the Ethernet device uses the
> > > bus layer to get the PHY.
> > > The Generic PHY Framework however doesn't have any bus layer. The PHY
> > > should be like any other Platform Devices and Drivers and the framework
> > > will provide some APIs to register with the framework. And there are
> > > other APIs which any controller can use to get the PHY (for e.g., in the
> > > case of dt boot, it can use phandle to get a reference to the PHY).
> >
> > Hmm, I think the use of a bus_type for a PHY actually sounds quite
> > appropriate. The actual PHY device would then be a child of the
>
> really ? I'm not so sure, the *bus* used by the PHY is ULPI, UTMI,
> UTMI+, PIP3, I2C, etc... adding another 'fake' bus representation is a
> bit overkill IMO.
>
> Imagine an I2C-controlled PHY driver like isp1301, that driver will have
> to register i2c_driver and phy_driver, which looks weird to me. If the
> only substitute for class is a bus, we can't drop classes just yet, I'm
> afraid.
>
> Imagine a regulator bus, a pwm bus, an LED bus etc. They don't make
> sense IMHO.
It's a fine line, but I think a phy is something that resembles a device
more than an LED does. When I read patch 1, I also noticed and commented
on the fact that it does use a 'class'. Now, according to Greg, we should
use 'bus_type' instead of 'class' in new code. I originally disagreed with
that concept, but he's the boss here and it's good if he has a vision
how things should be lined out.
In practice, there is little difference between a 'bus_type' and a 'class',
so just replace any instance of the former with the latter in your head
when reading the code ;-)
I understand that there is not a real common bus here, and the bus_type
infrastructure would basically be used as a way to represent each PHY
in sysfs and provide a way to enumerate and look them up inside of the
kernel.
Arnd
^ permalink raw reply
* Re: [PATCH v2 1/5] drivers: phy: add generic PHY framework
From: Arnd Bergmann @ 2013-02-19 14:28 UTC (permalink / raw)
To: kishon
Cc: rob, tony, linux, eballetbo, javier, balbi, gregkh, akpm, mchehab,
cesarb, davem, santosh.shilimkar, broonie, swarren, linux-doc,
linux-kernel, linux-arm-kernel, linux-omap, linux-usb, netdev
In-Reply-To: <51238485.1020408@ti.com>
On Tuesday 19 February 2013, kishon wrote:
> >> +
> >> + devname = dev_name(dev);
> >> + device_initialize(&phy->dev);
> >> + phy->desc = desc;
> >> + phy->dev.class = phy_class;
> >> + phy->dev.parent = dev;
> >> + phy->dev.bus = desc->bus;
> >> + ret = dev_set_name(&phy->dev, "%s", devname);
> >
> >
> > Passing a bus_type through the descriptor seems misplaced. What is this for?
>
> I thought if we are adding ethernet phys here (say drivers/phy/net), we
> can make phy_device_create() (currently in drivers/net/phy/phy_device.c)
> call phy_create with bus_type set to mdio_bus_type. Then we can have all
> the PHYs showing up in /sys/class/phy/ and ethernet can continue to use
> its own phy abstraction layer.
Hmm, that relies on the fact that mdio uses a 'bus_type' while the new phy
support uses a 'class', and it will break if we ever get to the point
where those two concepts are merged. I would rather not plan ahead here.
> > Why is this function not just:
> >
> > struct phy *phy_create(struct device *dev, const char *label, int type,
> > struct phy_ops *ops);
>
> since while calling the callback functions using ops, there wont be
> anyway to get back the device specific structure pointer.
>
> struct phy_dev {
> .
> .
> struct phy_descriptor desc;
> void __iomem *base;
> .
> .
> };
>
> static int phy_resume(struct phy_descriptor *desc)
> {
>
> //if we dont pass a member of phy_dev while *phy_create* we can't get
> back phy_dev from callback functions as used below.
> struct phy_dev *phy = desc_to_omapusb(desc);
>
> return 0;
> }
>
> static struct phy_ops ops = {
> .resume = phy_resume,
> .owner = THIS_MODULE,
> };
In other subsystems, that is what the device->private_data pointer is used
for, which you could also pass to phy_create, or set after calling that
function.
> > Passing a structure like you do here seems dangerous because when someone
> > decides to add members to the structure, existing code will not give a
> > build error but silently break.
>
> Not sure I understood this point. Care to explain?
Nevermind, when I wrote that sentence, I had not yet noticed that the
phy_descriptor is kept around. I was thinking that the structure was
only used to pass more arguments into phy_create.
Arnd
^ permalink raw reply
* Re: [PATCH net-next 2/3] tipc: byte-based overload control on socket receive queue
From: Neil Horman @ 2013-02-19 14:26 UTC (permalink / raw)
To: Jon Maloy; +Cc: Paul Gortmaker, David Miller, netdev, Ying Xue
In-Reply-To: <512332DA.5040508@ericsson.com>
On Tue, Feb 19, 2013 at 09:07:54AM +0100, Jon Maloy wrote:
> On 02/18/2013 09:47 AM, Neil Horman wrote:
> > On Fri, Feb 15, 2013 at 05:57:46PM -0500, Paul Gortmaker wrote:
> >> From: Ying Xue <ying.xue@windriver.com>
> >>
> >> Change overload control to be purely byte-based, using
> >> sk->sk_rmem_alloc as byte counter, and compare it to a calculated
> >> upper limit for the socket receive queue.
>
> [...]
>
> >> + *
> >> + * For all connectionless messages, by default new queue limits are
> >> + * as belows:
> >> + *
> >> + * TIPC_LOW_IMPORTANCE (5MB)
> >> + * TIPC_MEDIUM_IMPORTANCE (10MB)
> >> + * TIPC_HIGH_IMPORTANCE (20MB)
> >> + * TIPC_CRITICAL_IMPORTANCE (40MB)
> >> + *
> >> + * Returns overload limit according to corresponding message importance
> >> + */
> >> +static unsigned int rcvbuf_limit(struct sock *sk, struct sk_buff *buf)
> >> +{
> >> + struct tipc_msg *msg = buf_msg(buf);
> >> + unsigned int limit;
> >> +
> >> + if (msg_connected(msg))
> >> + limit = CONN_OVERLOAD_LIMIT;
> > This still strikes me as a bit wierd. If you really can't tolerate the default
> > rmem settings in proc, have you considered separating the rmem and wmem values
> > out into their own sysctls?
>
> Initially we tried to set this value as default for sk_rcvbuf, and then use
> fractions of it as limits, as you suggest below. The problem we found was that
> if we want to change this via setsockopt(SOL_SOCKET, SO_RCVBUF) the value range
> we can use is very limited, and doesn't fit our purposes.
>
Can you elaborate on this please? The above doesn't really explain why you
can't do what I suggested. Not asserting that what you say is untrue, mind you,
I'm just trying to understand what it is about TIPC that requires such a
specific reception buffer envelope, and how enforcing queue limits here is so
important when packets could just as easily be dropped at the ip layer (with
ostensibly no fatal failure).
> We did consider to introduce a separate setsockopt at TIPC level for this,
> but thought it had a value in itself to use the mechanism that is already there.
> Hence the "re-interpretation" of sk_rcvbuf as we do below.
> Considering the weird doubling of this parameter that is done elsewhere in the
> code we thought that having our own interpretation might be acceptable.
Thats quite different IMHO. The comments in sock_setsockopt make it pretty
clear that the doubling of the rcvbuf value is done to account for the sk_buff
overhead of packet reception, and thats documented in the socket(7) man page.
What you have here is the ability to set sk_rcvbuf, and then have that setting
be ignored, but only to within several different limits, depending on various
conditions, all of which are not visible to user space.
> We did of course see the potential issue with this, that is why we cc-ed
> you for comments.
I appreciate that.
> Now I see that David already pulled the series, so I am a little uncertain
> about how to proceed.
I saw that too, and asked him about this. A follow-on patch (if we wind up
deciding one is warranted) is the way to go here.
Regards
Neil
^ permalink raw reply
* Re: SYSFS "errors"
From: Felipe Balbi @ 2013-02-19 14:19 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Hannes Reinecke, Borislav Petkov, Felipe Balbi, Greg KH,
Linux Kernel Mailing List, Steven Rostedt, Frederic Weisbecker,
Ingo Molnar, JBottomley, linux-scsi, davem, netdev, Doug Thompson,
linux-edac, rjw, linux-pm
In-Reply-To: <20130219111440.2b4b2e95@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 513 bytes --]
Hi,
On Tue, Feb 19, 2013 at 11:14:40AM -0300, Mauro Carvalho Chehab wrote:
> > Please consider adding a default attribute which return '-EINVAL' or
> > somesuch when the function pointers are not set.
> > But _not_ adding it via device_create_file(). That's evil.
>
> This thread started with Felipe's complain about it returning -ENOSYS ;)
> when this feature is not supported.
I was complaining about a file with read permission not being really
readable which is a fair bug IMHO.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: SYSFS "errors"
From: Mauro Carvalho Chehab @ 2013-02-19 14:14 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Borislav Petkov, Felipe Balbi, Greg KH, Linux Kernel Mailing List,
Steven Rostedt, Frederic Weisbecker, Ingo Molnar, JBottomley,
linux-scsi, davem, netdev, Doug Thompson, linux-edac, rjw,
linux-pm
In-Reply-To: <512384EF.9020602@suse.de>
Em Tue, 19 Feb 2013 14:58:07 +0100
Hannes Reinecke <hare@suse.de> escreveu:
> On 02/19/2013 02:50 PM, Borislav Petkov wrote:
> > On Tue, Feb 19, 2013 at 03:38:09PM +0200, Felipe Balbi wrote:
> >> because changing the permission will cause the same issue:
> >
> > Actually, I take that back. Mauro's patch will already create the file
> > anyway:
> >
> > if (mci->set_sdram_scrub_rate || mci->get_sdram_scrub_rate)
> >
> > Adjusting the permissions is simply the last missing piece to this patch
> > to make the interface to userspace 100% coherent.
> >
> > --
> > From: Mauro Carvalho Chehab <mchehab@redhat.com>
> > Date: Tue, 19 Feb 2013 09:16:10 -0300
> > Subject: [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.
> >
> > If only read or only write is supported, it will keep returning -ENODEV
> > just like before.
> >
> > Reported-by: Felipe Balbi <balbi@ti.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> > ---
> > drivers/edac/edac_mc_sysfs.c | 21 +++++++++++++++++++--
> > 1 file changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> > index 0ca1ca71157f..5a788e60ff67 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
> > *
> > */
> > @@ -878,7 +878,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
> > };
> > @@ -1012,6 +1011,23 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci)
> > return err;
> > }
> >
> > + if (mci->set_sdram_scrub_rate || mci->get_sdram_scrub_rate) {
> > + umode_t mode = 0;
> > +
> > + if (mci->get_sdram_scrub_rate)
> > + mode = S_IRUGO;
> > +
> > + if (mci->set_sdram_scrub_rate)
> > + mode |= S_IWUSR;
> > +
> > + dev_attr_sdram_scrub_rate.attr.mode = mode;
> > +
> > + 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
> > */
> > @@ -1056,6 +1072,7 @@ fail:
> > continue;
> > device_unregister(&dimm->dev);
> > }
> > +fail2:
> > device_unregister(&mci->dev);
> > bus_unregister(&mci->bus);
> > kfree(mci->bus.name);
> >
> And of course you all know that creating sysfs attributes via
> 'device_create_file' opens all sort of funny race conditions,
> especially when checking these attributes from udev ...
Yes, we know that, but this subsystem has already lots of other
attributes created via device_create_file(). It used to be a
lot worse than that, as, on a very recent past (before Kernel 3.5),
those attributes were created via sysfs_create_file().
There's not much that can be done to avoid it on this subsystem.
>
> Please consider adding a default attribute which return '-EINVAL' or
> somesuch when the function pointers are not set.
> But _not_ adding it via device_create_file(). That's evil.
This thread started with Felipe's complain about it returning -ENOSYS ;)
when this feature is not supported.
Regards,
Mauro
^ permalink raw reply
* Re: SYSFS "errors"
From: Borislav Petkov @ 2013-02-19 14:10 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Felipe Balbi, Mauro Carvalho Chehab, Greg KH,
Linux Kernel Mailing List, Steven Rostedt, Frederic Weisbecker,
Ingo Molnar, JBottomley, linux-scsi, davem, netdev, Doug Thompson,
linux-edac, rjw, linux-pm
In-Reply-To: <512384EF.9020602@suse.de>
On Tue, Feb 19, 2013 at 02:58:07PM +0100, Hannes Reinecke wrote:
> Please consider adding a default attribute which return '-EINVAL' or
> somesuch when the function pointers are not set. But _not_ adding it
> via device_create_file(). That's evil.
This is what we do now. We probably could add the permissions fiddling
out in the the code but remain using DEVICE_ATTR.
Thanks.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
^ permalink raw reply
* Re: SYSFS "errors"
From: Hannes Reinecke @ 2013-02-19 13:58 UTC (permalink / raw)
To: Borislav Petkov, Felipe Balbi, Mauro Carvalho Chehab, Greg KH,
Linux Kernel Mailing List, Steven Rostedt, Frederic Weisbecker,
Ingo Molnar, JBottomley, linux-scsi, davem, netdev, Doug Thompson,
linux-edac, rjw, linux-pm
In-Reply-To: <20130219135056.GH26623@pd.tnic>
On 02/19/2013 02:50 PM, Borislav Petkov wrote:
> On Tue, Feb 19, 2013 at 03:38:09PM +0200, Felipe Balbi wrote:
>> because changing the permission will cause the same issue:
>
> Actually, I take that back. Mauro's patch will already create the file
> anyway:
>
> if (mci->set_sdram_scrub_rate || mci->get_sdram_scrub_rate)
>
> Adjusting the permissions is simply the last missing piece to this patch
> to make the interface to userspace 100% coherent.
>
> --
> From: Mauro Carvalho Chehab <mchehab@redhat.com>
> Date: Tue, 19 Feb 2013 09:16:10 -0300
> Subject: [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.
>
> If only read or only write is supported, it will keep returning -ENODEV
> just like before.
>
> Reported-by: Felipe Balbi <balbi@ti.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> ---
> drivers/edac/edac_mc_sysfs.c | 21 +++++++++++++++++++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> index 0ca1ca71157f..5a788e60ff67 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
> *
> */
> @@ -878,7 +878,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
> };
> @@ -1012,6 +1011,23 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci)
> return err;
> }
>
> + if (mci->set_sdram_scrub_rate || mci->get_sdram_scrub_rate) {
> + umode_t mode = 0;
> +
> + if (mci->get_sdram_scrub_rate)
> + mode = S_IRUGO;
> +
> + if (mci->set_sdram_scrub_rate)
> + mode |= S_IWUSR;
> +
> + dev_attr_sdram_scrub_rate.attr.mode = mode;
> +
> + 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
> */
> @@ -1056,6 +1072,7 @@ fail:
> continue;
> device_unregister(&dimm->dev);
> }
> +fail2:
> device_unregister(&mci->dev);
> bus_unregister(&mci->bus);
> kfree(mci->bus.name);
>
And of course you all know that creating sysfs attributes via
'device_create_file' opens all sort of funny race conditions,
especially when checking these attributes from udev ...
Please consider adding a default attribute which return '-EINVAL' or
somesuch when the function pointers are not set.
But _not_ adding it via device_create_file(). That's evil.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
^ permalink raw reply
* Re: [PATCH v2 1/5] drivers: phy: add generic PHY framework
From: kishon @ 2013-02-19 13:56 UTC (permalink / raw)
To: Arnd Bergmann
Cc: rob, tony, linux, eballetbo, javier, balbi, gregkh, akpm, mchehab,
cesarb, davem, santosh.shilimkar, broonie, swarren, linux-doc,
linux-kernel, linux-arm-kernel, linux-omap, linux-usb, netdev
In-Reply-To: <201302191256.24557.arnd@arndb.de>
Hi,
On Tuesday 19 February 2013 06:26 PM, Arnd Bergmann wrote:
> On Tuesday 19 February 2013, Kishon Vijay Abraham I wrote:
>> +static struct class *phy_class;
>> +static LIST_HEAD(phy_list);
>> +static DEFINE_MUTEX(phy_list_mutex);
>> +static LIST_HEAD(phy_bind_list);
>
> Hmm, so you actually do have a 'class'. There is a GregKH mandated ban on
> new classes, meaning that one should be converted to a bus_type instead.
>
> Also, you really should not need the global phy_list, phy_list_mutex
> and phy_bind_list variables, since the driver core already provides
> you with ways to iterate through devices on a class or bus.
ok.
>
>> +/**
>> + * of_phy_get - lookup and obtain a reference to a phy by phandle
>> + * @dev: device that requests this phy
>> + * @phandle: name of the property holding the phy phandle value
>> + * @index - the index of the phy
>> + *
>> + * Returns the phy associated with the given phandle value,
>> + * after getting a refcount to it or -ENODEV if there is no such phy or
>> + * -EPROBE_DEFER if there is a phandle to the phy, but the device is
>> + * not yet loaded.
>> + */
>> +struct phy *of_phy_get(struct device *dev, const char *phandle, u8 index)
>> +{
>> + struct phy *phy = NULL;
>> + struct phy_bind *phy_map = NULL;
>> + struct device_node *node;
>> +
>> + if (!dev->of_node) {
>> + dev_dbg(dev, "device does not have a device node entry\n");
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + node = of_parse_phandle(dev->of_node, phandle, index);
>> + if (!node) {
>> + dev_dbg(dev, "failed to get %s phandle in %s node\n", phandle,
>> + dev->of_node->full_name);
>> + return ERR_PTR(-ENODEV);
>> + }
>
> I wonder whether this one should be of_parse_phandle_with_args() instead,
> so you can have client-specific configuration in the property. Basically
> instead of
>
> phy = <&usbphy0 &usbphy1>;
>
> you can pass an arbitrary number of arguments along with this, as
> determined by some property in the phy node:
>
> usbphy0: phy@10000 {
> #phy-cells = <1>;
> };
>
> ehci@20000 {
> phy = <&usbphy0 23>;
> };
>
> Which in turn leads to the argument (23) being passed into a phy_bind().
>
> I also wonder if you should allow arbitrary names for the property.
> Can't this always be called 'phy'? If you allow named phys, it would
> more more consistent with other bindings to have a list of phy handles
> in a property called "phy", and a second property called "phy-names"
> that contains the named strings.
Ok. Makes sense. We should make both *phy* and *phy-cells* standard here.
>
>
>> +/**
>> + * phy_create - create a new phy
>> + * @dev: device that is creating the new phy
>> + * @desc: descriptor of the phy
>> + *
>> + * Called to create a phy using phy framework.
>> + */
>> +struct phy *phy_create(struct device *dev, struct phy_descriptor *desc)
>> +{
>> + int ret;
>> + struct phy *phy;
>> + struct phy_bind *phy_bind;
>> + const char *devname = NULL;
>> ...
>> +
>> + devname = dev_name(dev);
>> + device_initialize(&phy->dev);
>> + phy->desc = desc;
>> + phy->dev.class = phy_class;
>> + phy->dev.parent = dev;
>> + phy->dev.bus = desc->bus;
>> + ret = dev_set_name(&phy->dev, "%s", devname);
>
>
> Passing a bus_type through the descriptor seems misplaced. What is this for?
I thought if we are adding ethernet phys here (say drivers/phy/net), we
can make phy_device_create() (currently in drivers/net/phy/phy_device.c)
call phy_create with bus_type set to mdio_bus_type. Then we can have all
the PHYs showing up in /sys/class/phy/ and ethernet can continue to use
its own phy abstraction layer.
>
> Why is this function not just:
>
> struct phy *phy_create(struct device *dev, const char *label, int type,
> struct phy_ops *ops);
since while calling the callback functions using ops, there wont be
anyway to get back the device specific structure pointer.
struct phy_dev {
.
.
struct phy_descriptor desc;
void __iomem *base;
.
.
};
static int phy_resume(struct phy_descriptor *desc)
{
//if we dont pass a member of phy_dev while *phy_create* we can't get
back phy_dev from callback functions as used below.
struct phy_dev *phy = desc_to_omapusb(desc);
return 0;
}
static struct phy_ops ops = {
.resume = phy_resume,
.owner = THIS_MODULE,
};
>
> Passing a structure like you do here seems dangerous because when someone
> decides to add members to the structure, existing code will not give a
> build error but silently break.
Not sure I understood this point. Care to explain?
>
>> +/**
>> + * struct phy_ops - set of function pointers for performing phy operations
>> + * @init: operation to be performed for initializing phy
>> + * @exit: operation to be performed while exiting
>> + * @suspend: suspending the phy
>> + * @resume: resuming the phy
>> + * @poweron: powering on the phy
>> + * @shutdown: shutting down the phy
>> + * @owner: the module owner containing the ops
>> + */
>> +struct phy_ops {
>> + int (*init)(struct phy_descriptor *desc);
>> + int (*exit)(struct phy_descriptor *desc);
>> + int (*suspend)(struct phy_descriptor *desc);
>> + int (*resume)(struct phy_descriptor *desc);
>> + int (*poweron)(struct phy_descriptor *desc);
>> + int (*shutdown)(struct phy_descriptor *desc);
>> + struct module *owner;
>> +};
>
> Shouldn't these take the 'struct phy' as an argument? struct phy_descriptor is
> not even based on a 'struct device'.
I actually used struct phy_descriptor for the reason mentioned above.
Thanks a lot for reviewing.
Regards
Kishon
^ permalink raw reply
* Re: SYSFS "errors"
From: Borislav Petkov @ 2013-02-19 13:55 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: balbi, Greg KH, Linux Kernel Mailing List, Steven Rostedt,
Frederic Weisbecker, Ingo Molnar, JBottomley, linux-scsi, davem,
netdev, Doug Thompson, linux-edac, rjw, linux-pm
In-Reply-To: <20130219105048.198e2cfd@redhat.com>
On Tue, Feb 19, 2013 at 10:50:48AM -0300, Mauro Carvalho Chehab wrote:
> It is actually worse, as if someone is using a C code to open the device
> with
> fp = open ("/sys/devices/system/edac/mc/mc0/sdram_scrub_rate", O_RDWR);
>
> It will now start to fail if the device doesn't have both permissions.
That's fine - you're supposed to handle open()'s retval properly anyway. See my
other mail.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
^ permalink raw reply
* Re: SYSFS "errors"
From: Borislav Petkov @ 2013-02-19 13:50 UTC (permalink / raw)
To: Felipe Balbi
Cc: Mauro Carvalho Chehab, Greg KH, Linux Kernel Mailing List,
Steven Rostedt, Frederic Weisbecker, Ingo Molnar, JBottomley,
linux-scsi, davem, netdev, Doug Thompson, linux-edac, rjw,
linux-pm
In-Reply-To: <20130219133809.GA4390@arwen.pp.htv.fi>
On Tue, Feb 19, 2013 at 03:38:09PM +0200, Felipe Balbi wrote:
> because changing the permission will cause the same issue:
Actually, I take that back. Mauro's patch will already create the file
anyway:
if (mci->set_sdram_scrub_rate || mci->get_sdram_scrub_rate)
Adjusting the permissions is simply the last missing piece to this patch
to make the interface to userspace 100% coherent.
--
From: Mauro Carvalho Chehab <mchehab@redhat.com>
Date: Tue, 19 Feb 2013 09:16:10 -0300
Subject: [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.
If only read or only write is supported, it will keep returning -ENODEV
just like before.
Reported-by: Felipe Balbi <balbi@ti.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
drivers/edac/edac_mc_sysfs.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 0ca1ca71157f..5a788e60ff67 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
*
*/
@@ -878,7 +878,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
};
@@ -1012,6 +1011,23 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci)
return err;
}
+ if (mci->set_sdram_scrub_rate || mci->get_sdram_scrub_rate) {
+ umode_t mode = 0;
+
+ if (mci->get_sdram_scrub_rate)
+ mode = S_IRUGO;
+
+ if (mci->set_sdram_scrub_rate)
+ mode |= S_IWUSR;
+
+ dev_attr_sdram_scrub_rate.attr.mode = mode;
+
+ 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
*/
@@ -1056,6 +1072,7 @@ fail:
continue;
device_unregister(&dimm->dev);
}
+fail2:
device_unregister(&mci->dev);
bus_unregister(&mci->bus);
kfree(mci->bus.name);
--
1.8.1.3.535.ga923c31
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
^ permalink raw reply related
* Re: SYSFS "errors"
From: Mauro Carvalho Chehab @ 2013-02-19 13:50 UTC (permalink / raw)
To: balbi
Cc: Borislav Petkov, Greg KH, Linux Kernel Mailing List,
Steven Rostedt, Frederic Weisbecker, Ingo Molnar, JBottomley,
linux-scsi, davem, netdev, Doug Thompson, linux-edac, rjw,
linux-pm
In-Reply-To: <20130219133809.GA4390@arwen.pp.htv.fi>
[-- Attachment #1: Type: text/plain, Size: 1178 bytes --]
Em Tue, 19 Feb 2013 15:38:09 +0200
Felipe Balbi <balbi@ti.com> escreveu:
> Hi,
>
> On Tue, Feb 19, 2013 at 02:28:54PM +0100, Borislav Petkov wrote:
> > On Tue, Feb 19, 2013 at 03:15:00PM +0200, Felipe Balbi wrote:
> > > what's the problem with that ?
> >
> > Not a problem - simply annoying.
> >
> > $ ./test.sh
> > cat: /path/to/file/that/doesnt/exist.txt: No such file or directory
> > Setting scrubrate to:
> >
> > I'm sure someone would ask themselves why all of a sudden the file is
> > gone.
> >
> > > $ cat /path/to/file/that/doesnt/exist.txt
> > > cat: /path/to/file/that/doesnt/exist.txt: No such file or directory
> > >
> > > Didn't see any gates to hell opening here...
> >
> > And I don't see why we have to debate such a trivial thing when we can
> > fix it *properly* without *absolutely* *not* upsetting userspace.
>
> because changing the permission will cause the same issue:
It is actually worse, as if someone is using a C code to open the device
with
fp = open ("/sys/devices/system/edac/mc/mc0/sdram_scrub_rate", O_RDWR);
It will now start to fail if the device doesn't have both permissions.
Regards,
Mauro
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [Xen-devel] [PATCH] xen: netback: remove redundant xenvif_put
From: Ian Campbell @ 2013-02-19 13:47 UTC (permalink / raw)
To: Jan Beulich
Cc: David Miller, drjones@redhat.com, xen-devel@lists.xensource.com,
linux-kernel@vger.kernel.org, netdev@vger.kernel.org
In-Reply-To: <51233FF502000078000BF4AE@nat28.tlf.novell.com>
On Tue, 2013-02-19 at 08:03 +0000, Jan Beulich wrote:
> The core of
> it is that the put here parallels the one in netbk_tx_err(), and
> the one in xenvif_carrier_off() matches the get from
> xenvif_connect() (which normally would be done on the path
> coming through xenvif_disconnect()).
A few people have made this mistake already, perhaps this helps make it
more obvious what is going on...
8<--------------------------------
>From 7b93077e2cda5881b594d9c7e733e617df87d7c0 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Tue, 19 Feb 2013 10:46:12 +0000
Subject: [PATCH] xen/netback: refactor xenvif_carrier_on from xenvif_connect
Several people have been confused by the vif reference count taken by
xenvif_connect() being dropped in xenvif_carrier_off(). Factor out bringing
the carrier up (and the associated reference grab) to try and make the
relationship more obvious.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
drivers/net/xen-netback/interface.c | 49 +++++++++++++++++++---------------
1 files changed, 27 insertions(+), 22 deletions(-)
diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index d984141..059d726 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -307,6 +307,32 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
return vif;
}
+static void xenvif_carrier_on(struct xenvif *vif)
+{
+ xenvif_get(vif);
+
+ rtnl_lock();
+ if (!vif->can_sg && vif->dev->mtu > ETH_DATA_LEN)
+ dev_set_mtu(vif->dev, ETH_DATA_LEN);
+ netdev_update_features(vif->dev);
+ netif_carrier_on(vif->dev);
+ if (netif_running(vif->dev))
+ xenvif_up(vif);
+ rtnl_unlock();
+}
+
+void xenvif_carrier_off(struct xenvif *vif)
+{
+ struct net_device *dev = vif->dev;
+
+ rtnl_lock();
+ netif_carrier_off(dev); /* discard queued packets */
+ if (netif_running(dev))
+ xenvif_down(vif);
+ rtnl_unlock();
+ xenvif_put(vif);
+}
+
int xenvif_connect(struct xenvif *vif, unsigned long tx_ring_ref,
unsigned long rx_ring_ref, unsigned int evtchn)
{
@@ -328,16 +354,7 @@ int xenvif_connect(struct xenvif *vif, unsigned long tx_ring_ref,
vif->irq = err;
disable_irq(vif->irq);
- xenvif_get(vif);
-
- rtnl_lock();
- if (!vif->can_sg && vif->dev->mtu > ETH_DATA_LEN)
- dev_set_mtu(vif->dev, ETH_DATA_LEN);
- netdev_update_features(vif->dev);
- netif_carrier_on(vif->dev);
- if (netif_running(vif->dev))
- xenvif_up(vif);
- rtnl_unlock();
+ xenvif_carrier_on(vif);
return 0;
err_unmap:
@@ -346,18 +363,6 @@ err:
return err;
}
-void xenvif_carrier_off(struct xenvif *vif)
-{
- struct net_device *dev = vif->dev;
-
- rtnl_lock();
- netif_carrier_off(dev); /* discard queued packets */
- if (netif_running(dev))
- xenvif_down(vif);
- rtnl_unlock();
- xenvif_put(vif);
-}
-
void xenvif_disconnect(struct xenvif *vif)
{
if (netif_carrier_ok(vif->dev))
--
1.7.2.5
^ permalink raw reply related
* Re: SYSFS "errors"
From: Mauro Carvalho Chehab @ 2013-02-19 13:42 UTC (permalink / raw)
To: Borislav Petkov
Cc: balbi, Greg KH, Linux Kernel Mailing List, Steven Rostedt,
Frederic Weisbecker, Ingo Molnar, JBottomley, linux-scsi, davem,
netdev, Doug Thompson, linux-edac, rjw, linux-pm
In-Reply-To: <20130219130626.GE26623@pd.tnic>
Em Tue, 19 Feb 2013 14:06:26 +0100
Borislav Petkov <bp@alien8.de> escreveu:
> > No, on both cases, open() will return an error (-ENOENT against -EPERM).
>
> What if it is a shell script doing:
>
> cat /sys/devices/system/edac/mc/mc0/sdram_scrub_rate
>
> or similar?
Well, cat will return "1" if an error is found, no matter what error happened.
With an existing file (-ENOSYS):
$ cat /sys/devices/system/edac/mc/mc0/sdram_scrub_rate; echo $?
cat: /sys/devices/system/edac/mc/mc0/sdram_scrub_rate: No such device
1
When the file doesn't exist (-ENOENT):
$ cat /sys/devices/system/edac/mc/mc0/sdram_scrub_rate_not_exist; echo $?
cat: /sys/devices/system/edac/mc/mc0/sdram_scrub_rate_not_exist: No such file or directory
1
When permission doesn't match (-EPERM):
$ cat /sys/devices/system/cpu/probe; echo $?
cat: /sys/devices/system/cpu/probe: Permission denied
1
When everything is ok, it will return 0
$ cat /sys/devices/system/edac/mc/mc0/ce_count; echo $? >/dev/null
0
A script ready to handle -ENOSYS would be doing, instead:
RATE=$(cat /sys/devices/system/edac/mc/mc0/ce_count 2>/dev/null)
if [ "$?" == "0" ]; then echo "Scrub rate: $RATE"; fi
So, a bash script won't notice any difference.
The only difference will be noticed if the script is written on some other
language and the script is dumb enough to assume success if the errno is
different than -ENOSYS.
--
Cheers,
Mauro
^ permalink raw reply
* Re: SYSFS "errors"
From: Felipe Balbi @ 2013-02-19 13:38 UTC (permalink / raw)
To: Borislav Petkov, Felipe Balbi, Mauro Carvalho Chehab, Greg KH,
Linux Kernel Mailing List, Steven Rostedt, Frederic Weisbecker,
Ingo Molnar, JBottomley, linux-scsi, davem, netdev, Doug Thompson,
linux-edac, rjw, linux-pm
In-Reply-To: <20130219132853.GG26623@pd.tnic>
[-- Attachment #1: Type: text/plain, Size: 1141 bytes --]
Hi,
On Tue, Feb 19, 2013 at 02:28:54PM +0100, Borislav Petkov wrote:
> On Tue, Feb 19, 2013 at 03:15:00PM +0200, Felipe Balbi wrote:
> > what's the problem with that ?
>
> Not a problem - simply annoying.
>
> $ ./test.sh
> cat: /path/to/file/that/doesnt/exist.txt: No such file or directory
> Setting scrubrate to:
>
> I'm sure someone would ask themselves why all of a sudden the file is
> gone.
>
> > $ cat /path/to/file/that/doesnt/exist.txt
> > cat: /path/to/file/that/doesnt/exist.txt: No such file or directory
> >
> > Didn't see any gates to hell opening here...
>
> And I don't see why we have to debate such a trivial thing when we can
> fix it *properly* without *absolutely* *not* upsetting userspace.
because changing the permission will cause the same issue:
$ cat temporary.txt
cat: temporary.txt: Permission denied
$ ls -l temporary.txt
---------- 1 balbi balbi 0 Feb 19 15:37 temporary.txt
$ cat temporary.txt
cat: temporary.txt: Permission denied
$ cat non-existent.txt
cat: non-existent.txt: No such file or directory
just a different error code is returned...
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: SYSFS "errors"
From: Borislav Petkov @ 2013-02-19 13:28 UTC (permalink / raw)
To: Felipe Balbi
Cc: Mauro Carvalho Chehab, Greg KH, Linux Kernel Mailing List,
Steven Rostedt, Frederic Weisbecker, Ingo Molnar, JBottomley,
linux-scsi, davem, netdev, Doug Thompson, linux-edac, rjw,
linux-pm
In-Reply-To: <20130219131500.GW23197@arwen.pp.htv.fi>
On Tue, Feb 19, 2013 at 03:15:00PM +0200, Felipe Balbi wrote:
> what's the problem with that ?
Not a problem - simply annoying.
$ ./test.sh
cat: /path/to/file/that/doesnt/exist.txt: No such file or directory
Setting scrubrate to:
I'm sure someone would ask themselves why all of a sudden the file is
gone.
> $ cat /path/to/file/that/doesnt/exist.txt
> cat: /path/to/file/that/doesnt/exist.txt: No such file or directory
>
> Didn't see any gates to hell opening here...
And I don't see why we have to debate such a trivial thing when we can
fix it *properly* without *absolutely* *not* upsetting userspace.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
^ permalink raw reply
* Re: SYSFS "errors"
From: Felipe Balbi @ 2013-02-19 13:15 UTC (permalink / raw)
To: Borislav Petkov, Mauro Carvalho Chehab, balbi, Greg KH,
Linux Kernel Mailing List, Steven Rostedt, Frederic Weisbecker,
Ingo Molnar, JBottomley, linux-scsi, davem, netdev, Doug Thompson,
linux-edac, rjw, linux-pm
In-Reply-To: <20130219130626.GE26623@pd.tnic>
[-- Attachment #1: Type: text/plain, Size: 1102 bytes --]
On Tue, Feb 19, 2013 at 02:06:26PM +0100, Borislav Petkov wrote:
> On Tue, Feb 19, 2013 at 09:46:40AM -0300, Mauro Carvalho Chehab wrote:
> > Ah, now I see what you're meaning. That would require to dynamically
> > create a per-mci DEVICE_ATTR().
>
> Dude, look at the code, we do that already. And you're using it with
> dev_attr_sdram_scrub_rate.
>
> Simply change the permissions of the attribute before calling
> device_create_file.
>
> pseudo:
>
> umode_t mode = 0;
>
> if (mci->set...)
> mode |= S_IWUSR;
>
> if (mci->get...)
> mode |= S_IRUGO;
>
>
> dev_attr_sdram_scrub_rate.attr.mode = mode;
>
> device_create_file(&mci->dev, &dev_attr_sdram_scrub_rate);
>
> > No, on both cases, open() will return an error (-ENOENT against -EPERM).
>
> What if it is a shell script doing:
>
> cat /sys/devices/system/edac/mc/mc0/sdram_scrub_rate
what's the problem with that ?
$ cat /path/to/file/that/doesnt/exist.txt
cat: /path/to/file/that/doesnt/exist.txt: No such file or directory
Didn't see any gates to hell opening here...
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: Fw: [Bug 54021] New: No internet with kernel 3.7 or 3.8 with LAN
From: J. K. Cliburn @ 2013-02-19 13:13 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Chris Snook, netdev, nic-devel, Huang, Xiong
In-Reply-To: <20130218220552.4d54c9a9@nehalam.linuxnetplumber.net>
Xiong,
Can you please investigate this atl1c issue?
Jay
On 02/19/2013 12:05 AM, Stephen Hemminger wrote:
> Also, in link it references the devices
>
> $ lspci -nnk | grep -i net -A2
> 01:00.0 Ethernet controller [0200]: Atheros Communications Inc. AR8152 v1.1 Fast Ethernet [1969:2060] (rev c1)
> Subsystem: Acer Incorporated [ALI] Device [1025:0349]
> Kernel driver in use: atl1c
> --
> 02:00.0 Network controller [0280]: Atheros Communications Inc. AR9285 Wireless Network Adapter (PCI-Express) [168c:002b] (rev 01)
> Subsystem: Lite-On Communications Inc Device [11ad:6631]
> Kernel driver in use: ath9k
>
> Begin forwarded message:
>
> Date: Sun, 17 Feb 2013 05:38:37 -0800
> From: "bugzilla-daemon@bugzilla.kernel.org" <bugzilla-daemon@bugzilla.kernel.org>
> To: "stephen@networkplumber.org" <stephen@networkplumber.org>
> Subject: [Bug 54021] New: No internet with kernel 3.7 or 3.8 with LAN
>
>
> https://bugzilla.kernel.org/show_bug.cgi?id=54021
>
> Summary: No internet with kernel 3.7 or 3.8 with LAN
> Product: Networking
> Version: 2.5
> Kernel Version: since 3.7
> Platform: All
> OS/Version: Linux
> Tree: Mainline
> Status: NEW
> Severity: normal
> Priority: P1
> Component: IPV4
> AssignedTo: shemminger@linux-foundation.org
> ReportedBy: david.erbs@gmail.com
> Regression: No
>
>
> Hello since kernel update 3.7 or 3.8 I have no internet connection with LAN,
> wlan is working very well, with lan my internet connection is working for 5
> minutes, then internet is not working anymore. I have to unplug the cable and
> the i have to plug it again - so it will work 5 minutes.....
>
> I have a netbook (acer aspire one d255)
>
>
> With kernel 3.5x everything is working fine.
>
> Problem is reported here (German)
>
> http://forum.ubuntuusers.de/topic/internet-verbindung-bricht-ab/
>
> If you need more information, feel free to ask
>
>
> kind regards
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox