| Message ID | 1390208555-27770-3-git-send-email-tushar.behera@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 20, 2014 at 02:32:35PM +0530, Tushar Behera wrote: > uart_register_driver call binds the driver to a specific device > node through tty_register_driver call. This should typically happen > during device probe call. > > In a multiplatform scenario, it is possible that multiple serial > drivers are part of the kernel. Currently the driver registration fails > if multiple serial drivers with same default major/minor numbers are > included in the kernel. > > A typical case is observed with amba-pl011 and samsung-uart drivers. NAK. There should not be any other driver using amba-pl011's device numbers.
On Mon, Jan 20, 2014 at 10:04:15AM +0000, Russell King - ARM Linux wrote: > On Mon, Jan 20, 2014 at 02:32:35PM +0530, Tushar Behera wrote: > > uart_register_driver call binds the driver to a specific device > > node through tty_register_driver call. This should typically happen > > during device probe call. > > > > In a multiplatform scenario, it is possible that multiple serial > > drivers are part of the kernel. Currently the driver registration fails > > if multiple serial drivers with same default major/minor numbers are > > included in the kernel. > > > > A typical case is observed with amba-pl011 and samsung-uart drivers. > > NAK. There should not be any other driver using amba-pl011's device numbers. I agree, but there is. And because of that, moving the registration to the probe call fixes the issue with building a kernel with all of the drivers built into them, so I'm going to take both of these patches, as it does solve that problem, while still allowing the device number collision to happen. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 13, 2014 at 10:12:16AM -0800, Greg KH wrote: > On Mon, Jan 20, 2014 at 10:04:15AM +0000, Russell King - ARM Linux wrote: > > On Mon, Jan 20, 2014 at 02:32:35PM +0530, Tushar Behera wrote: > > > uart_register_driver call binds the driver to a specific device > > > node through tty_register_driver call. This should typically happen > > > during device probe call. > > > > > > In a multiplatform scenario, it is possible that multiple serial > > > drivers are part of the kernel. Currently the driver registration fails > > > if multiple serial drivers with same default major/minor numbers are > > > included in the kernel. > > > > > > A typical case is observed with amba-pl011 and samsung-uart drivers. > > > > NAK. There should not be any other driver using amba-pl011's device numbers. > > I agree, but there is. And because of that, moving the registration to > the probe call fixes the issue with building a kernel with all of the > drivers built into them, so I'm going to take both of these patches, as > it does solve that problem, while still allowing the device number > collision to happen. So what happens when two _devices_ are probed by this driver at the same time?
On Thu, Feb 13, 2014 at 06:15:59PM +0000, Russell King - ARM Linux wrote: > On Thu, Feb 13, 2014 at 10:12:16AM -0800, Greg KH wrote: > > On Mon, Jan 20, 2014 at 10:04:15AM +0000, Russell King - ARM Linux wrote: > > > On Mon, Jan 20, 2014 at 02:32:35PM +0530, Tushar Behera wrote: > > > > uart_register_driver call binds the driver to a specific device > > > > node through tty_register_driver call. This should typically happen > > > > during device probe call. > > > > > > > > In a multiplatform scenario, it is possible that multiple serial > > > > drivers are part of the kernel. Currently the driver registration fails > > > > if multiple serial drivers with same default major/minor numbers are > > > > included in the kernel. > > > > > > > > A typical case is observed with amba-pl011 and samsung-uart drivers. > > > > > > NAK. There should not be any other driver using amba-pl011's device numbers. > > > > I agree, but there is. And because of that, moving the registration to > > the probe call fixes the issue with building a kernel with all of the > > drivers built into them, so I'm going to take both of these patches, as > > it does solve that problem, while still allowing the device number > > collision to happen. > > So what happens when two _devices_ are probed by this driver at the same > time? The bus that the driver is on will not allow that to happen, I thought we went through this before... And yes, devices on different busses could cause problems, but is that the case here for these devices? At first glance, I don't think that can happen for these drivers. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 13, 2014 at 10:27:01AM -0800, Greg KH wrote: > On Thu, Feb 13, 2014 at 06:15:59PM +0000, Russell King - ARM Linux wrote: > > On Thu, Feb 13, 2014 at 10:12:16AM -0800, Greg KH wrote: > > > On Mon, Jan 20, 2014 at 10:04:15AM +0000, Russell King - ARM Linux wrote: > > > > On Mon, Jan 20, 2014 at 02:32:35PM +0530, Tushar Behera wrote: > > > > > uart_register_driver call binds the driver to a specific device > > > > > node through tty_register_driver call. This should typically happen > > > > > during device probe call. > > > > > > > > > > In a multiplatform scenario, it is possible that multiple serial > > > > > drivers are part of the kernel. Currently the driver registration fails > > > > > if multiple serial drivers with same default major/minor numbers are > > > > > included in the kernel. > > > > > > > > > > A typical case is observed with amba-pl011 and samsung-uart drivers. > > > > > > > > NAK. There should not be any other driver using amba-pl011's device numbers. > > > > > > I agree, but there is. And because of that, moving the registration to > > > the probe call fixes the issue with building a kernel with all of the > > > drivers built into them, so I'm going to take both of these patches, as > > > it does solve that problem, while still allowing the device number > > > collision to happen. > > > > So what happens when two _devices_ are probed by this driver at the same > > time? > > The bus that the driver is on will not allow that to happen, I thought > we went through this before... > > And yes, devices on different busses could cause problems, but is that > the case here for these devices? At first glance, I don't think that > can happen for these drivers. We went through this before, and I stated the paths, and no one disagreed with that. It /is/ racy.
On Thu, Feb 13, 2014 at 06:42:49PM +0000, Russell King - ARM Linux wrote: > On Thu, Feb 13, 2014 at 10:27:01AM -0800, Greg KH wrote: > > On Thu, Feb 13, 2014 at 06:15:59PM +0000, Russell King - ARM Linux wrote: > > > On Thu, Feb 13, 2014 at 10:12:16AM -0800, Greg KH wrote: > > > > On Mon, Jan 20, 2014 at 10:04:15AM +0000, Russell King - ARM Linux wrote: > > > > > On Mon, Jan 20, 2014 at 02:32:35PM +0530, Tushar Behera wrote: > > > > > > uart_register_driver call binds the driver to a specific device > > > > > > node through tty_register_driver call. This should typically happen > > > > > > during device probe call. > > > > > > > > > > > > In a multiplatform scenario, it is possible that multiple serial > > > > > > drivers are part of the kernel. Currently the driver registration fails > > > > > > if multiple serial drivers with same default major/minor numbers are > > > > > > included in the kernel. > > > > > > > > > > > > A typical case is observed with amba-pl011 and samsung-uart drivers. > > > > > > > > > > NAK. There should not be any other driver using amba-pl011's device numbers. > > > > > > > > I agree, but there is. And because of that, moving the registration to > > > > the probe call fixes the issue with building a kernel with all of the > > > > drivers built into them, so I'm going to take both of these patches, as > > > > it does solve that problem, while still allowing the device number > > > > collision to happen. > > > > > > So what happens when two _devices_ are probed by this driver at the same > > > time? > > > > The bus that the driver is on will not allow that to happen, I thought > > we went through this before... > > > > And yes, devices on different busses could cause problems, but is that > > the case here for these devices? At first glance, I don't think that > > can happen for these drivers. > > We went through this before, and I stated the paths, and no one disagreed > with that. > > It /is/ racy. Ok, I just went and looked at the uart driver register path, and I don't see the race (note, if there is one, it's there today, regardless of this patch). uart_register_driver() fils in a bunch of fields, and passes control off to tty_register_driver(). If the minor/major number allocation here fails, due to collisions, then an error occurs, and things back out safely. If the allocation succeeds, then we lock the list of drivers, add them to the list, then register the tty device with the subsystem for how ever many tty devices were asked for. Yes, the locking might be wrong here, in the tty layer, but again, that's nothing new created by this patch. So I fail to see the problem with applying this patch, as it solves a problem people are having, and should be fine given that no hardware with both of these devices will ever be present at the same time. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 13, 2014 at 03:26:06PM -0800, Greg KH wrote: > On Thu, Feb 13, 2014 at 06:42:49PM +0000, Russell King - ARM Linux wrote: > > We went through this before, and I stated the paths, and no one disagreed > > with that. > > > > It /is/ racy. > > Ok, I just went and looked at the uart driver register path, and I don't > see the race (note, if there is one, it's there today, regardless of > this patch). The race isn't the uart code, it's the driver model. Consider what happens when this happens: * Two pl011 devices get registered at the same time by two different threads. * Both devices have a lock taken on the _device_ itself before matching against the driver. * Both devices get matched to the same driver. * Both devices are passed into the driver's probe function. * Both check uart_reg.state, both call uart_register_driver() on that at the same time, which results in two allocations inside uart_register_driver(), one gets overwritten... So, the /only/ thing which stops this happening is that the devices are generally available before the driver is registered, and driver registration results in devices being probed serially. Moreover, both attempt to call tty_register_driver()... one succeeds, the other fails. However, what about the userspace bind/unbind methods. Yes, userspace can ask the driver core to unbind devices from a driver or bind - and again, there's no per-driver locking here. So, if you can trigger two concurrent binds from userspace, you hit the same race as above. So, if you want to accept these patches, go ahead, introduce races, but personally I'd recommend plugging these races.
On Fri, Feb 14, 2014 at 12:07:17AM +0000, Russell King - ARM Linux wrote: > On Thu, Feb 13, 2014 at 03:26:06PM -0800, Greg KH wrote: > > On Thu, Feb 13, 2014 at 06:42:49PM +0000, Russell King - ARM Linux wrote: > > > We went through this before, and I stated the paths, and no one disagreed > > > with that. > > > > > > It /is/ racy. > > > > Ok, I just went and looked at the uart driver register path, and I don't > > see the race (note, if there is one, it's there today, regardless of > > this patch). > > The race isn't the uart code, it's the driver model. > > Consider what happens when this happens: > > * Two pl011 devices get registered at the same time by two different > threads. How? What two different busses will see this same device? The amba bus code should prevent that from happening, right? If not, there's bigger problems in that bus code :) That's where this problem should be fixed, if there is one, otherwise this same issue would be there for any type of driver that calles into the uart core, right? > * Both devices have a lock taken on the _device_ itself before matching > against the driver. > > * Both devices get matched to the same driver. > > * Both devices are passed into the driver's probe function. > > * Both check uart_reg.state, both call uart_register_driver() on that > at the same time, which results in two allocations inside > uart_register_driver(), one gets overwritten... > > So, the /only/ thing which stops this happening is that the devices > are generally available before the driver is registered, and driver > registration results in devices being probed serially. Moreover, both > attempt to call tty_register_driver()... one succeeds, the other fails. > > However, what about the userspace bind/unbind methods. Yes, userspace > can ask the driver core to unbind devices from a driver or bind - and > again, there's no per-driver locking here. So, if you can trigger two > concurrent binds from userspace, you hit the same race as above. > > So, if you want to accept these patches, go ahead, introduce races, but > personally I'd recommend plugging these races. The only way to solve this would be to do it in the bus, I don't see anything here that makes it any "racier" than it currently is. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 13, 2014 at 04:14:36PM -0800, Greg KH wrote: > On Fri, Feb 14, 2014 at 12:07:17AM +0000, Russell King - ARM Linux wrote: > > On Thu, Feb 13, 2014 at 03:26:06PM -0800, Greg KH wrote: > > > On Thu, Feb 13, 2014 at 06:42:49PM +0000, Russell King - ARM Linux wrote: > > > > We went through this before, and I stated the paths, and no one disagreed > > > > with that. > > > > > > > > It /is/ racy. > > > > > > Ok, I just went and looked at the uart driver register path, and I don't > > > see the race (note, if there is one, it's there today, regardless of > > > this patch). > > > > The race isn't the uart code, it's the driver model. > > > > Consider what happens when this happens: > > > > * Two pl011 devices get registered at the same time by two different > > threads. > > How? What two different busses will see this same device? The amba bus > code should prevent that from happening, right? If not, there's bigger > problems in that bus code :) Where is that requirement documented? It isn't documented. No one implements any kind of locking at the bus level to prevent this, not PCI, nor platform devices. > That's where this problem should be fixed, if there is one, otherwise > this same issue would be there for any type of driver that calles into > the uart core, right? And how does bus code prevent this? By intercepting the driver model ->probe callback and taking its own lock maybe? Really? What about those buses which don't wrap the probe callback?
> > How? What two different busses will see this same device? The amba bus > > code should prevent that from happening, right? If not, there's bigger > > problems in that bus code :) > > Where is that requirement documented? It isn't documented. No one > implements any kind of locking at the bus level to prevent this, not > PCI, nor platform devices. We don't want bus locking. There are lots of busses that can be parallel probed and in some cases its expensive not to do so. We may well need to do much more parallel probing in PCI bus in future and we may be parallel probing multiple bus types at once. The uart_register hack is simply broken. Nobody can stop you merging it Greg but in the long term its the wrong approach. We've identified a correct working approach which is to simply add a CONFIG entry to the ARM tree and a few ifdefs to the problem drivers to make the "problem" (in fact a complete fictional non-problem) go away and to get rid of the mess over time completely as the drivers are set dynamic and it turns out that all the userspace happens to already handle this just fine. Far better than botchfixes to uart registration code that will haunt us for years. Alan -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 17, 2014 at 03:35:18PM +0000, One Thousand Gnomes wrote: > We've identified a correct working approach which is to simply add a > CONFIG entry to the ARM tree and a few ifdefs to the problem drivers to > make the "problem" (in fact a complete fictional non-problem) go away and > to get rid of the mess over time completely as the drivers are set > dynamic and it turns out that all the userspace happens to already handle > this just fine. It's a very real problem which affects actual kernels that distro style users are building.
On Tue, 18 Feb 2014 08:50:13 +0900 Mark Brown <broonie@kernel.org> wrote: > On Mon, Feb 17, 2014 at 03:35:18PM +0000, One Thousand Gnomes wrote: > > > We've identified a correct working approach which is to simply add a > > CONFIG entry to the ARM tree and a few ifdefs to the problem drivers to > > make the "problem" (in fact a complete fictional non-problem) go away and > > to get rid of the mess over time completely as the drivers are set > > dynamic and it turns out that all the userspace happens to already handle > > this just fine. > > It's a very real problem which affects actual kernels that distro style > users are building. Only because you persist in trying to keep the old static minor numbers even though they are not needed by anything in the real world that will ever run such kernels. Also only because you are apparently too slack to bother to check whether the driver matches the platform or there is an device tree node before registering it even though that's trivial to code and is already done by some other platforms and serial drivers just fine. It's a core code "problem" that is invented by refusing to do the simple trivial fixes in the ARM tree. Please clean up your own poop instead of trying to shovel it into the street. And the proposed change set is buggy as hell - because we register things like 8250 devices at least four ways on the same x86 machine all of which could in theory occur in parallel. Alan -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > The race isn't the uart code, it's the driver model. > > > > Consider what happens when this happens: > > > > * Two pl011 devices get registered at the same time by two different > > threads. > > How? What two different busses will see this same device? The amba bus > code should prevent that from happening, right? If not, there's bigger > problems in that bus code :) > > That's where this problem should be fixed, if there is one, otherwise > this same issue would be there for any type of driver that calles into > the uart core, right? I did some more digging into this the problem goes beyond the uart and driver model code. Even if you serialize every bus and you serialize every bus against every other bus (which you can't do btw as we get recursive probes of one bus from a device probe of another) it's still racy. We have non bus registering paths for some drivers and those could be user triggered. An obvious example is 8250. If the 8250 driver is loaded on a platform where no device is autodetected (which is true on a few wackier PC laptops with touchscreens on an 8250) then you hae a race between say a PCMCIA card insertion of an 8250 and a user who creates the first 8250 device using setserial at the same moment. There are similar races between the various directly created devices on some of the serial ports and bus probed ones where the driver mixes bus probing with olde worlde straight forward hardcoding. Alan -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 18, 2014 at 10:09:43AM +0000, Etched Pixels wrote: > Mark Brown <broonie@kernel.org> wrote: > > It's a very real problem which affects actual kernels that distro style > > users are building. > Only because you persist in trying to keep the old static minor > numbers even though they are not needed by anything in the real world > that will ever run such kernels. Also only because you are apparently > too slack to bother to check whether the driver matches the platform or > there is an device tree node before registering it even though that's > trivial to code and is already done by some other platforms and serial > drivers just fine. > It's a core code "problem" that is invented by refusing to do the simple > trivial fixes in the ARM tree. Please clean up your own poop instead of > trying to shovel it into the street. Please try to avoid this sort of misdirected yelling, it's not helping anything to complain that people are following the recommendations of the maintainer or to demand that this somehow gets hacked around in arch/ when we're trying to convince all the architectures to get their drivers merged into subsystem trees so they're reviewed by the subsystem maintainers. If you have some examples of better practice for serial device registration that you'd like to identify that'd be helpful, but what you're doing here is making disparaging remarks and telling people to adopt bad practices because someone else made a mistake a decade ago. That is how problems like this get created in the first place. > And the proposed change set is buggy as hell - because we register things > like 8250 devices at least four ways on the same x86 machine all of which > could in theory occur in parallel. Then you need to convince Greg of that. The most recent set of patches are exactly what he asked for.
> Please try to avoid this sort of misdirected yelling, it's not helping I don't see any misdirected yelling > anything to complain that people are following the recommendations of > the maintainer or to demand that this somehow gets hacked around in > arch/ when we're trying to convince all the architectures to get their > drivers merged into subsystem trees so they're reviewed by the subsystem > maintainers. It's not a case of hacking around it in arch. It's a case of fixing up problems where they belong, which btw is what Greg has in his tree - specifically ef2889f7ffee67f0aed49e854c72be63f1466759 and 6f134c3c770355b7e930d3ffc558864668f13055 which keep the handling of the minor clash cock-up in the drivers affected. No garbage in the core tty drivers, no racy delayed registration in the core uart code. In the longer term we should probably just abolish the fixed minor numbers, nothing in the real world cares about them enough to justify keeping them forever. > you're doing here is making disparaging remarks and telling people to > adopt bad practices because someone else made a mistake a decade ago. I'm not telling anyone to adopt bad practices - at least its news to me that "stopping the merge of buggy crap" is now a bad practice. > That is how problems like this get created in the first place. We were having a discussion about the fact the proposed patch for deferring the processing was buggy, and probably unfixably so. That's a code correctness discussion. Merging overcomplicated buggy crap causes problems. > > And the proposed change set is buggy as hell - because we register things > > like 8250 devices at least four ways on the same x86 machine all of which > > could in theory occur in parallel. > > Then you need to convince Greg of that. The most recent set of patches > are exactly what he asked for. No there's an assumption that when someone asks for patches the proposed changes actually *work*. As Russell has demonstrated - the general deferral patches for the uart/tty layer are broken. The driver specific fixups in -next on the other hand appear to be fine providing the amba bus probe remains serialized (and trivially fixed if it doesn't). Alan -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 19, 2014 at 02:47:51PM +0000, One Thousand Gnomes wrote: > > anything to complain that people are following the recommendations of > > the maintainer or to demand that this somehow gets hacked around in > > arch/ when we're trying to convince all the architectures to get their > > drivers merged into subsystem trees so they're reviewed by the subsystem > > maintainers. > It's not a case of hacking around it in arch. It's a case of fixing up > problems where they belong, which btw is what Greg has in his tree - > specifically ef2889f7ffee67f0aed49e854c72be63f1466759 and > 6f134c3c770355b7e930d3ffc558864668f13055 which keep the handling of the > minor clash cock-up in the drivers affected. That's more reasonable, what you were saying was to do things in the ARM tree which reads like you want changes in arch/arm - those changes are in the serial code. > > you're doing here is making disparaging remarks and telling people to > > adopt bad practices because someone else made a mistake a decade ago. > I'm not telling anyone to adopt bad practices - at least its news to me > that "stopping the merge of buggy crap" is now a bad practice. Like I say, it's the "do it in the ARM tree" bit that's bad practice. > > > And the proposed change set is buggy as hell - because we register things > > > like 8250 devices at least four ways on the same x86 machine all of which > > > could in theory occur in parallel. > > Then you need to convince Greg of that. The most recent set of patches > > are exactly what he asked for. > No there's an assumption that when someone asks for patches the proposed > changes actually *work*. As Russell has demonstrated - the general > deferral patches for the uart/tty layer are broken. The driver specific > fixups in -next on the other hand appear to be fine providing the amba > bus probe remains serialized (and trivially fixed if it doesn't). So like I say discuss that with Greg, it's entirely reasonable for a submitter to trust the maintainer on something like this so it's not helpful to yell at them (or other people who happen to be working on the same architecture for that matter).
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index d58783d..d4eda24 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -2154,9 +2154,19 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id) amba_ports[i] = uap; amba_set_drvdata(dev, uap); + + if (!amba_reg.state) { + ret = uart_register_driver(&amba_reg); + if (ret < 0) { + pr_err("Failed to register AMBA-PL011 driver\n"); + return ret; + } + } + ret = uart_add_one_port(&amba_reg, &uap->port); if (ret) { amba_ports[i] = NULL; + uart_unregister_driver(&amba_reg); pl011_dma_remove(uap); } out: @@ -2175,6 +2185,7 @@ static int pl011_remove(struct amba_device *dev) amba_ports[i] = NULL; pl011_dma_remove(uap); + uart_unregister_driver(&amba_reg); return 0; } @@ -2230,22 +2241,14 @@ static struct amba_driver pl011_driver = { static int __init pl011_init(void) { - int ret; printk(KERN_INFO "Serial: AMBA PL011 UART driver\n"); - ret = uart_register_driver(&amba_reg); - if (ret == 0) { - ret = amba_driver_register(&pl011_driver); - if (ret) - uart_unregister_driver(&amba_reg); - } - return ret; + return amba_driver_register(&pl011_driver); } static void __exit pl011_exit(void) { amba_driver_unregister(&pl011_driver); - uart_unregister_driver(&amba_reg); } /*
uart_register_driver call binds the driver to a specific device node through tty_register_driver call. This should typically happen during device probe call. In a multiplatform scenario, it is possible that multiple serial drivers are part of the kernel. Currently the driver registration fails if multiple serial drivers with same default major/minor numbers are included in the kernel. A typical case is observed with amba-pl011 and samsung-uart drivers. Signed-off-by: Tushar Behera <tushar.behera@linaro.org> --- drivers/tty/serial/amba-pl011.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)