lkml.org 
[lkml]   [2022]   [Dec]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v7 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550.


On Tue, 20 Dec 2022, Andy Shevchenko wrote:

> On Tue, Dec 20, 2022 at 08:36:52AM -0800, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Add a Device Feature List (DFL) bus driver for the Altera
>> 16550 implementation of UART.
>
> In general the code here looks good to me, but one thing to discuss due to
> comment to the previous patch(es).
>
> ...
>
>> + u64 *p;
>> +
>> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
>> + if (!p)
>> + return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ param\n");
>> +
>> + p++;
>> + uart->port.uartclk = *p;
>
> So, here and the below is using always the second u64 from the returned data.
> Does it mean:
> - we always skip the first u64 from the returned buffer and hence... (see below)

The first u64 of the parameter block, the parameter header, contains a
version field and a next/size field that a parameter consumer might use.
The version field determines the exact layout of the data, and the
next/size field could/should be used to prevent out of bounds accesses.

> - we may actually return the second u64 as a plain number (not a pointer) from
> (an additional?) API? In such case we would not need to take care about this
> p++; lines here and there.

I think an additional API that can be used to fetch an array of u64's
while also checking boundary conditions would be helpful.

> - we have fixed length of the data, returned by find_param(), i.e. 2 u64 words?

The length and layout of the parameter data is determined by the parameter
id and version. So the data portion of a parameter is not fixed length.

Thanks for the feedback,
Matthew Gerlach

>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>

\
 
 \ /
  Last update: 2023-03-26 23:17    [W:0.062 / U:0.192 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site