linux-kernel - Re: [PATCH] tty/serial: at91: restore dynamic driver binding

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Wed, 24 Feb 2016 16:32:53 +0100
From:	romain izard <romain.izard@...ile-devices.fr>
To:	Nicolas Ferre <nicolas.ferre@...el.com>
Cc:	linux-serial@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Paul Gortmaker <paul.gortmaker@...driver.com>,
	Jiri Slaby <jslaby@...e.com>
Subject: Re: [PATCH] tty/serial: at91: restore dynamic driver binding

2016-02-24 15:53 GMT+01:00 Nicolas Ferre <nicolas.ferre@...el.com>:
> Le 23/02/2016 17:59, Romain Izard a écrit :
>> In commit c39dfebc7798956fd2140ae6321786ff35da30c3, the modular support
>> code for atmel_serial was removed, as the driver cannot be built as a
>> module. Because no use case was proposed, the dynamic driver binding
>> support was removed as well.
>>
>> The atmel_serial driver can manage up to 7 serial controllers, which are
>> multiplexed with other functions. For example, in the Atmel SAMA5D2, the
>> Flexcom controllers can work as USART, SPI or I2C controllers, and on
>> all Atmel devices serial lines can be reconfigured as GPIOs.
>
> Well this paragraph somehow puzzled me and made me think that you only
> have to keep the serial port as "disabled" in the DT to achieve what you
> had had in mind.
>
>> My use case uses GPIOs to transfer a firmware update using a custom
>> protocol on the lines used as a serial port during the normal life of
>> the device. If it is not possible to unbind the atmel_serial driver, the
>> GPIO lines remain reserved and prevent this case from working.
>
> Yes, here I understand better. Your use case is somewhat uncommon as
> your SoC pads can be configured for two different uses with two
> different drivers in front of your hardware device...
>
>> This patch reinstates the atmel_serial_remove function, and fixes it as
>> it failed to clear the "clk" field on removal, triggering an oops when
>> a device was bound again after being unbound.
>
> Well, okay. As the modification is not that big and that the solution is
> pretty elegant, I'll take it.
>
>
>> Signed-off-by: Romain Izard <romain.izard.pro@...il.com>
>> ---
>>  drivers/tty/serial/atmel_serial.c | 30 +++++++++++++++++++++++++++---
>>  1 file changed, 27 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
>> index 1c0884d8ef32..59e241723edc 100644
>> --- a/drivers/tty/serial/atmel_serial.c
>> +++ b/drivers/tty/serial/atmel_serial.c
>> @@ -2759,14 +2759,38 @@ err:
>>       return ret;
>>  }
>>
>> +static int atmel_serial_remove(struct platform_device *pdev)
>> +{
>> +     struct uart_port *port = platform_get_drvdata(pdev);
>> +     struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
>> +     int ret = 0;
>> +
>> +     tasklet_kill(&atmel_port->tasklet);
>> +
>> +     device_init_wakeup(&pdev->dev, 0);
>> +
>> +     ret = uart_remove_one_port(&atmel_uart, port);
>> +
>> +     kfree(atmel_port->rx_ring.buf);
>> +
>> +     /* "port" is allocated statically, so we shouldn't free it */
>> +
>> +     clear_bit(port->line, atmel_ports_in_use);
>> +
>> +     clk_put(atmel_port->clk);
>> +     atmel_port->clk = NULL;
>> +
>> +     return ret;
>> +}
>> +
>>  static struct platform_driver atmel_serial_driver = {
>>       .probe          = atmel_serial_probe,
>> +     .remove         = atmel_serial_remove,
>>       .suspend        = atmel_serial_suspend,
>>       .resume         = atmel_serial_resume,
>>       .driver         = {
>> -             .name                   = "atmel_usart",
>> -             .of_match_table         = of_match_ptr(atmel_serial_dt_ids),
>> -             .suppress_bind_attrs    = true,
>> +             .name   = "atmel_usart",
>> +             .of_match_table = of_match_ptr(atmel_serial_dt_ids),
>
> The 2 modifications above are not related to the patch: keep them like
> they were event if it's not as pretty as you would like...
>

These modifications were introduced by the patch I reversed, so the
alignment just returned to what it was before Paul's patch. And I need
to remove "suppress_bind_attrs", otherwise it's not possible to unbind
the device and the driver.

Best regards,
-- 
Romain Izard