Messages in this thread | | | Date | Wed, 21 Dec 2022 14:16:38 -0800 (PST) | From | matthew.gerlach@linux ... | Subject | Re: [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 > > >
| |