| 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
| ||
|
Message-ID: <223d7807-fd46-6bf7-1211-1ad5223e75c3@suse.de>
Date: Mon, 3 Jul 2017 00:36:28 +0200
From: Andreas Färber <afaerber@...e.de>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: support@...aker.org,
张天益 <tyzhang@...ions-semi.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
96boards@...obotics.com,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Thomas Liau <thomas.liau@...ions-semi.com>,
mp-cs@...ions-semi.com,
"linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
刘炜 <liuwei@...ions-semi.com>,
Jiri Slaby <jslaby@...e.com>,
linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
张东风 <zhangdf@...ions-semi.com>,
梅利 <harrymei@...ions-semi.com>,
support@...ietech.com, lee@...ietech.com
Subject: Re: [PATCH v4 16/28] tty: serial: owl: Implement console driver
Am 07.06.2017 um 16:37 schrieb Andy Shevchenko:
> On Tue, Jun 6, 2017 at 3:54 AM, Andreas Färber <afaerber@...e.de> wrote:
>> Implement serial console driver to complement earlycon.
>>
>> Based on LeMaker linux-actions tree.
>
>> +#define OWL_UART_CTL_DWLS_MASK (0x3 << 0)
>
> GENMASK() ?
>
>> +#define OWL_UART_CTL_PRS_MASK (0x7 << 4)
>
> Ditto.
>
>> #define OWL_UART_STAT_TRFL_MASK (0x1f << 11)
>
> Ditto.
Changed.
>> +static struct owl_uart_port *owl_uart_ports[OWL_UART_PORT_NUM];
>
> And this is needed for...?
That's what both the downstream driver and several in-tree drivers are
doing. See `git grep '_ports\[' -- drivers/tty/serial/`.
If you feel this is wrong, --verbose explanation please.
>> +static void owl_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
>> +{
>> +}
>
> Do we need an empty stub?
The only flag I have found in the CTL register is AFE for automatic
RTS/CTS flow-control - RTS and CTS can only be read in STAT, not set.
And yes, if I drop this empty callback function, I get no serial output.
>> +static void owl_uart_send_chars(struct uart_port *port)
>> +{
>
>> + xmit->tail = (xmit->tail + 1) & (SERIAL_XMIT_SIZE - 1);
>
> % SERIAL_XMIT_SIZE shorter (I hope it's a power of 2), but it's up to you.
crisv10 and meson_uart have it this way, modulo normalized whitespace.
No serial driver uses % for it.
>> +}
>
>> +static irqreturn_t owl_uart_irq(int irq, void *dev_id)
>> +{
>> + struct uart_port *port = (struct uart_port *)dev_id;
>
> Useless casting.
Fixed.
>> + spin_lock(&port->lock);
>
> spin_lock_irqsave() ?
Fixed, with matching _irqrestore().
> Consider the kernel command option that switches all IRQ handlers to
> be threaded.
>
>> +}
>
>> +static void owl_uart_change_baudrate(struct owl_uart_port *owl_port,
>> + unsigned long baud)
>> +{
>> + clk_set_rate(owl_port->clk, baud * 8);
>> +}
>
>> +static void owl_uart_release_port(struct uart_port *port)
>> +{
>> + struct platform_device *pdev = to_platform_device(port->dev);
>> + struct resource *res;
>> +
>
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res)
>> + return;
>> +
>> + if (port->flags & UPF_IOREMAP) {
>> + devm_release_mem_region(port->dev, port->mapbase,
>> + resource_size(res));
>> + devm_iounmap(port->dev, port->membase);
>> + port->membase = NULL;
>> + }
>
> Above is entirely redundant.
Can you explain what this flag is used for and why some other drivers
implement it? That word alone is not helping.
>> +}
>> +
>> +static int owl_uart_request_port(struct uart_port *port)
>> +{
>> + struct platform_device *pdev = to_platform_device(port->dev);
>> + struct resource *res;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res)
>> + return -ENXIO;
>> +
>> + if (!devm_request_mem_region(port->dev, port->mapbase,
>> + resource_size(res), dev_name(port->dev)))
>> + return -EBUSY;
>> +
>> + if (port->flags & UPF_IOREMAP) {
>> + port->membase = devm_ioremap_nocache(port->dev, port->mapbase,
>> + resource_size(res));
>> + if (!port->membase)
>> + return -EBUSY;
>> + }
>> +
>> + return 0;
>> +}
>
>> +static void owl_uart_config_port(struct uart_port *port, int flags)
>> +{
>
>> + if (flags & UART_CONFIG_TYPE) {
>
> if (!(...))
> return;
>
> ?
Not a single serial driver does it that way.
I guess it prepares for handling other flags.
>> + port->type = PORT_OWL;
>> + owl_uart_request_port(port);
>> + }
>> +}
>
>
>> +static int owl_uart_probe(struct platform_device *pdev)
>> +{
>> + struct resource *res_mem, *res_irq;
>> + struct owl_uart_port *owl_port;
>> + int ret;
>> +
>> + if (pdev->dev.of_node)
>> + pdev->id = of_alias_get_id(pdev->dev.of_node, "serial");
>> +
>> + if (pdev->id < 0 || pdev->id >= OWL_UART_PORT_NUM) {
>> + dev_err(&pdev->dev, "id %d out of range\n", pdev->id);
>> + return -EINVAL;
>> + }
>> +
>
>
>> + res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res_mem) {
>> + dev_err(&pdev->dev, "could not get mem\n");
>> + return -ENODEV;
>> + }
>
> You can use
>
> struct resource *mem;
>
> mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> x = devm_ioremap_resource();
> if (IS_ERR(x))
> return PTR_ERR(x);
>
> and remote IOREMAP flag below.
>> + res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> + if (!res_irq) {
>> + dev_err(&pdev->dev, "could not get irq\n");
>> + return -ENODEV;
>> + }
>
> platform_get_irq()
Adopted.
>> + if (owl_uart_ports[pdev->id]) {
>> + dev_err(&pdev->dev, "port %d already allocated\n", pdev->id);
>> + return -EBUSY;
>> + }
>
> I guess it's redundant. It should be conflicting by resource (for example, IRQ).
No, currently not. Memory resources are allocated on request_port, IRQ
is requested on startup.
>> + owl_port->clk = clk_get(&pdev->dev, NULL);
>
> devm_ ?
Changed.
>> + owl_port->port.iotype = UPIO_MEM;
>> + owl_port->port.mapbase = res_mem->start;
>> + owl_port->port.irq = res_irq->start;
>
>
>> + owl_port->port.uartclk = clk_get_rate(owl_port->clk);
>
> You need to check if it's 0 and use device property instead or bail out.
Fixed. Since this is a new driver, I'd rather not fall back to
clock-frequency property here. The binding has clocks property as required.
>> + owl_port->port.flags = UPF_BOOT_AUTOCONF | UPF_IOREMAP | UPF_LOW_LATENCY;
>> + owl_port->port.x_char = 0;
>> + owl_port->port.fifosize = 16;
>> + owl_port->port.ops = &owl_uart_ops;
>> +
>> + owl_uart_ports[pdev->id] = owl_port;
>
>> + platform_set_drvdata(pdev, owl_port);
>
> It should be just before return 0, right?..
Does it matter?
>> +
>> + ret = uart_add_one_port(&owl_uart_driver, &owl_port->port);
>> + if (ret)
>> + owl_uart_ports[pdev->id] = NULL;
>
> ...and thus, taking into consideration redundancy of that global var:
>
> ret = uart_add_one_port(&owl_uart_driver, &owl_port->port);
> if (ret)
> retrun ret;
>
> platform_set_drvdata(pdev, owl_port);
> return 0;
>
>> + return ret;
>> +}
>
>> +
>> +static int owl_uart_remove(struct platform_device *pdev)
>> +{
>
>> + struct owl_uart_port *owl_port;
>> +
>> + owl_port = platform_get_drvdata(pdev);
>
> Do above in one line.
Done.
>> + uart_remove_one_port(&owl_uart_driver, &owl_port->port);
>
>> + owl_uart_ports[pdev->id] = NULL;
>
> Redundant.
>
>> +
>> + return 0;
>> +}
>
>> +/* Actions Semi Owl UART */
>> +#define PORT_OWL 117
>
> We can use holes for now IIUC.
>
> See 36 or alike
Okay. 36 works, too.
Please point to a good example of a serial driver that is not
"redundant" in your view. That will help also with other pending serial
drivers of mine. Thanks.
Regards,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Powered by blists - more mailing lists