On 21-04-14 14:14:34, Konrad Rzeszutek Wilk wrote:
> > +static int cdat_to_buffer(struct pcie_doe *doe, u32 *buffer, size_t length)
> > +{
> > + int entry_handle = 0;
> > + int rc;
> > +
> > + do {
> > + u32 cdat_request_pl = CDAT_DOE_REQ(entry_handle);
> > + u32 cdat_response_pl[32];
> > + struct pcie_doe_exchange ex = {
> > + .vid = PCI_DVSEC_VENDOR_ID_CXL,
> > + .protocol = CXL_DOE_PROTOCOL_TABLE_ACCESS,
> > + .request_pl = &cdat_request_pl,
> > + .request_pl_sz = sizeof(cdat_request_pl),
> > + .response_pl = cdat_response_pl,
> > + .response_pl_sz = sizeof(cdat_response_pl),
> > + };
> > + size_t entry_dw;
> > + u32 *entry;
> > +
> > + rc = pcie_doe_sync(doe, &ex);
> > + if (rc < 0)
> > + return rc;
> > +
> > + entry = cdat_response_pl + 1;
> > + entry_dw = rc / sizeof(u32);
>
> Could you >> 2?
What's the reasoning for >> 2? As someone who regularly does this because of
habit, using divide is generally a lot more implicitly descriptive.
> > + /* Skip Header */
> > + entry_dw -= 1;
> > + entry_dw = min(length / 4, entry_dw);
>
> Ditto here on length?
>
> Also should you double check that length is not greater than
> sizeof(cdat_respone_pl)?
>
> > + memcpy(buffer, entry, entry_dw * sizeof(u32));
> > + length -= entry_dw * sizeof(u32);
>
> Would it be just easier to convert length at the start to be >> 2
> instead of this * and / ?
>
Same as above. Assuming the compiler isn't braindead, I think it reads better as
it is.
>
> > + buffer += entry_dw;
> > + entry_handle = FIELD_GET(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE, cdat_response_pl[0]);
> > +
> > + } while (entry_handle != 0xFFFF);
> > +
> > + return 0;
> > +}
> > +
