On 28 December 2013 00:14, Greg KH gregkh@linuxfoundation.org wrote:
On Fri, Dec 27, 2013 at 03:47:31PM +0530, Tushar Behera wrote:
On 27 December 2013 12:08, Greg KH gregkh@linuxfoundation.org wrote:
On Fri, Dec 27, 2013 at 12:00:20PM +0530, Tushar Behera wrote:
On 27 December 2013 10:48, Greg KH gregkh@linuxfoundation.org wrote:
On Fri, Dec 27, 2013 at 10:37:28AM +0530, Tushar Behera wrote:
[ ... ]
@@ -951,8 +949,6 @@ static struct uart_driver s3c24xx_uart_drv = { .nr = CONFIG_SERIAL_SAMSUNG_UARTS, .cons = S3C24XX_SERIAL_CONSOLE, .dev_name = S3C24XX_SERIAL_NAME,
.major = S3C24XX_SERIAL_MAJOR,.minor = S3C24XX_SERIAL_MINOR,Doesn't this break existing systems and configurations that are expecting 204:64 as the location of this serial port?
I tested this on Exynos4210-Origen, Exynos5250-Arndale board, it works fine there. I haven't tested on any older boards.
How did it work? You are relying on some userspace tools to do this properly, right? What about systems without those specific tools?
Enabling CONFIG_DEVTMPFS, all the /dev/ttySAC<n> nodes are generated and the appropriate console is specified through command line argument.
But what about systems that rely on a hard-coded /dev?
Look, I'm all for making everyone use devtmpfs, but just changing major:minor numbers for drivers isn't ok, as you are changing the userspace ABI for the device.
Please realize what you are asking for here, I really don't think you grasp it given that you didn't ask any of the maintainers of this driver about the change in the first place.
Please get approval for this patch from others within Linaro before sending it out again. Linaro has a process in place for this type of thing, please use it, otherwise it makes people like me really grumpy and upset and causes me to yell at people at their conferences.
greg k-h
Asking for opinion about this ... Without this patch, I can't get serial console to come up on Exynos platforms when amba-pl011 driver enabled. But Greg is particularly not at all happy with this patch. Any comments with respect to fixing this issue would be highly appreciated.
Attaching the complete patch for reference.
On Mon, Dec 30, 2013 at 08:21:43AM +0530, Tushar Behera wrote:
On 28 December 2013 00:14, Greg KH gregkh@linuxfoundation.org wrote:
But what about systems that rely on a hard-coded /dev?
Look, I'm all for making everyone use devtmpfs, but just changing major:minor numbers for drivers isn't ok, as you are changing the
Please realize what you are asking for here, I really don't think you grasp it given that you didn't ask any of the maintainers of this driver about the change in the first place.
Please get approval for this patch from others within Linaro before sending it out again. Linaro has a process in place for this type of thing, please use it, otherwise it makes people like me really grumpy and upset and causes me to yell at people at their conferences.
Asking for opinion about this ... Without this patch, I can't get serial console to come up on Exynos platforms when amba-pl011 driver enabled. But Greg is particularly not at all happy with this patch. Any comments with respect to fixing this issue would be highly appreciated.
The most direct thing would seem to be to provide a fallback in the serial core which (perhaps optionally) assigns a dynamic number only if there's a conflict as an alternative to failing registration; it won't break existing systems since they wouldn't have been able to register the extra device in the first place.
On 3 January 2014 16:44, Mark Brown broonie@kernel.org wrote:
On Mon, Dec 30, 2013 at 08:21:43AM +0530, Tushar Behera wrote:
On 28 December 2013 00:14, Greg KH gregkh@linuxfoundation.org wrote:
But what about systems that rely on a hard-coded /dev?
Look, I'm all for making everyone use devtmpfs, but just changing major:minor numbers for drivers isn't ok, as you are changing the
Please realize what you are asking for here, I really don't think you grasp it given that you didn't ask any of the maintainers of this driver about the change in the first place.
Please get approval for this patch from others within Linaro before sending it out again. Linaro has a process in place for this type of thing, please use it, otherwise it makes people like me really grumpy and upset and causes me to yell at people at their conferences.
Asking for opinion about this ... Without this patch, I can't get serial console to come up on Exynos platforms when amba-pl011 driver enabled. But Greg is particularly not at all happy with this patch. Any comments with respect to fixing this issue would be highly appreciated.
The most direct thing would seem to be to provide a fallback in the serial core which (perhaps optionally) assigns a dynamic number only if there's a conflict as an alternative to failing registration; it won't break existing systems since they wouldn't have been able to register the extra device in the first place.
Mark,
Thanks for your suggestion. Let me rework my patch based on this approach.
In a multi-platform scenario, the hard-coded major/minor numbers in serial drivers may conflict with each other. A typical scenario is observed with amba-pl011 and samsung-serial drivers, both of these drivers use same set of major/minor numbers. Because of this there is no serial output on Samsung boards if amba-pl011 is enabled alongwith samsung-serial driver.
The issue is fixed by adding a fallback in driver core, so that we can use dynamic major number in case device node allocation fails for hard-coded major/minor number.
Signed-off-by: Tushar Behera tushar.behera@linaro.org --- drivers/tty/tty_io.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c index c74a00a..4ed35b9 100644 --- a/drivers/tty/tty_io.c +++ b/drivers/tty/tty_io.c @@ -3341,6 +3341,18 @@ int tty_register_driver(struct tty_driver *driver) dev_t dev; struct device *d;
+ if (driver->major) { + dev = MKDEV(driver->major, driver->minor_start); + error = register_chrdev_region(dev, driver->num, driver->name); + /* In case of error, fall back to dynamic allocation */ + if (error < 0) + driver->major = 0; + } + + /* + * Don't replace the following check with an else to above if statement, + * as it may also be called as a fallback. + */ if (!driver->major) { error = alloc_chrdev_region(&dev, driver->minor_start, driver->num, driver->name); @@ -3348,9 +3360,6 @@ int tty_register_driver(struct tty_driver *driver) driver->major = MAJOR(dev); driver->minor_start = MINOR(dev); } - } else { - dev = MKDEV(driver->major, driver->minor_start); - error = register_chrdev_region(dev, driver->num, driver->name); } if (error < 0) goto err;
On Mon, Jan 13, 2014 at 03:52:16PM +0530, Tushar Behera wrote:
- if (driver->major) {
dev = MKDEV(driver->major, driver->minor_start);error = register_chrdev_region(dev, driver->num, driver->name);/* In case of error, fall back to dynamic allocation */if (error < 0)driver->major = 0;- }
This probably needs to at least print a warning if the major number is being overridden.
On 13 January 2014 17:13, Mark Brown broonie@kernel.org wrote:
On Mon, Jan 13, 2014 at 03:52:16PM +0530, Tushar Behera wrote:
if (driver->major) {dev = MKDEV(driver->major, driver->minor_start);error = register_chrdev_region(dev, driver->num, driver->name);/* In case of error, fall back to dynamic allocation */if (error < 0)driver->major = 0;}This probably needs to at least print a warning if the major number is being overridden.
Ok. I will update the patch with following print.
printk(KERN_WARNING "Device major number for %s is overridden\n", driver->name);
Thanks for reviewing.
On Mon, Jan 13, 2014 at 05:32:44PM +0530, Tushar Behera wrote:
On 13 January 2014 17:13, Mark Brown broonie@kernel.org wrote:
This probably needs to at least print a warning if the major number is being overridden.
Ok. I will update the patch with following print.
printk(KERN_WARNING "Device major number for %s is overridden\n", driver->name);
Thanks for reviewing.
The user might be interested to know what the original value was (and the new one but that's a bit more invasive).
Might also be useful to have this feature be optional though I'd see what Greg says.
linaro-kernel@lists.linaro.org
-
Mark Brown -
Tushar Behera