linux-kernel - Re: [PATCH] Respond:add support for SUNIX Multi-I/O board

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]

Message-ID: <a2758c4b-ac49-e731-46b0-6990c3534da7@metux.net>
Date:   Fri, 22 Mar 2019 19:26:46 +0100
From:   "Enrico Weigelt, metux IT consult" <lkml@...ux.net>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Morris Ku <saumah@...il.com>, gregkh <gregkh@...uxfoundation.org>,
        morris_ku@...ix.com,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Respond:add support for SUNIX Multi-I/O board

On 21.03.19 09:53, Arnd Bergmann wrote:

> I think serial port drivers typically have a limit on the number of
> ports, so we likely need the same here. 

Except for earlycon (haven't had a closer look at it yet), I don't see
any hard reason for that (maybe Greg could give us more insight here).
I seriously doubt that these boards could be relevant for earlycon.
(hard to find a machine that needs serial earlycon while not having some
uart on-board ;-))

> However the number of boards should not be limited in the mfd driver
> that just exports the serial ports to the serial driver.

Exactly. And there should be any global arrays for these boards, where
every access runs through. Or even worse: looping through it, to find
the right entry, within ISRs.

>>> +> +static int snx_ser_port_total_cnt;
>>> +> +static int snx_par_port_total_cnt;
>>> +> +
>>> +> +int snx_board_count;
>>> +> +
>>> +> +static struct snx_ser_driver sunix_ser_reg = {
>>> +> +     .dev_name = "ttySNX",
>>> +> +     .major = 0,
>>> +> +     .minor = 0,
>>> +> +     .nr = (SNX_SER_TOTAL_MAX + 1),
>>> +> +};
>>> +
>>> +can't this be const ?
>>> +
>>> i prefer keep it in current format.
>>
>> Why ? do you ever have to write into this struct ?
> 
> I think this will just go away after the rewrite into a proper
> layered driver, so it doesn't matter.

That's exactly the point, which I've been repeating over and over again.

By the way, one more weird thing that slipped trough my previous
reviews: he's redefining standard structs and typecasting them.
Actually, he copy+paste'd large parts of the serial and parport
subsystems.

> The best way I can think of to do this would be to have a nested
> irqchip in the mfd driver that handles the per-board interrupt and
> provides a unique irq number to the individual port drivers.
> This makes a very simple irq handler in the port driver, and keeps
> the multiplexing where it belongs.

Exactly. That's the way it works w/ lot's of other composite devices.
(I recall some exotic daq cards that work similar way)

@Morris: please try to find a fitting generic, configurable irqchip
driver first. If you don't find one, talk to us and give us a detailed
explaination how that part of your cards actually works, so we could
write some generic driver. I might have some customer specific devices
that could also benefit from that.

--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@...ux.net -- +49-151-27565287