linux-kernel - Re: [PATCH 1/1] xilinx ps uart: Adding a kernel parameter for the number of xilinx ps uarts

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 24 May 2017 15:06:13 +0200
From:   Michal Simek <michal.simek@...inx.com>
To:     Alan Cox <gnomes@...rguk.ukuu.org.uk>,
        Michal Simek <michal.simek@...inx.com>
CC:     Rob Herring <robh+dt@...nel.org>,
        Sam Povilus <kernel.development@...il.us>,
        <gregkh@...uxfoundation.org>, <jslaby@...e.com>,
        <soren.brinkmann@...inx.com>, <linux-serial@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] xilinx ps uart: Adding a kernel parameter for the
 number of xilinx ps uarts

On 23.5.2017 22:07, Alan Cox wrote:
>> yep hardcoded max 4 where in probe first free space is found and used
>> (range 0-3) but still max3100s statically allocated.
>> Shouldn't be this also dynamically allocated?
> 
> The code to do the dynamic allocation would be larger than the array of
> pointers for the sane worst case.
> 
>> I am not quite sure how exactly you want to do this via DT.
> 
> Count the number of DT entries for this kind of port and allocate that
> many ?

:-) I think there is no doubt how to calculate that number for
static/fixed system. But where do you want to put it? And who should
read it?

A lot of drivers are calling uart_register_driver in module_init.
Others are calling it in probe(pl011 for example).

In module init you probably don't have a pointer to DT to read this and
this should be in generic location not in node itself.
If all drivers should call it from probe then we should change that -
then reading property is easy and only location should be cleared.

That's why I am curious how exactly you would do it because I haven't
seen this before.

> 
>>
>> Also what do you think is a safe maximum number? This is fpga - hundreds
>> of pins which can do just uart.
>>
>>> There are lots of options better than breaking the "one kernel many
>>> platforms" model.  
>>
>> Another options is also module parameter and dynamically allocated array
>> in cdns_uart_init.
> 
> For a given platform the number is constant and they need to be
> described, so it seems to make no sense to put it anywhere other than the
> DT for that platform.

Also I don't think this is really constant in all cases. Especially in
connection to fpga where you can put others IPs to PL. You never know
what it is present in partial region and how many of these are there.
It means having truly dynamic behavior would be welcome.

Can you call uart_register_driver() from probe for every instance? It
means nr is 1 all the time.

> 
> Why should users have to pass magic config options not use DT as
> intended ?

I am not saying that config option is perfect solution. It is at least
aligned with 5 others serial drivers in the tree.

Thanks,
Michal