On Fri, Oct 04, 2024 at 10:47:01PM +0900, Damien Le Moal wrote: > On 10/4/24 21:11, Niklas Cassel wrote: (snip) > > I think that you need to also include the patch that implements map_align() > > for rk3399 in this series (without any other rk3399 patches), such that the > > API actually makes sense. > > This is coming in a followup series. Note that the Cadence controller also look > suspiciously similar to the rk3399, so it may need the same treatment. I strongly think that you should continue to include patch: "PCI: rockchip-ep: Implement the map_align endpoint controller operation" (that was part of your V2) in this series. (Just do not include any of the other random fixes for rockchip-ep that were part of your V2.) Because without an implementer of the API that can actually return a size smaller than requested, the current API makes no sense. We do send out a series that provides a complex API if there is no implementer (in that same series) which require the complexity. The single implementer in this series (DWC PCIe EP), does not require the complexity of the API. (The reason for this is that it is possible (for whatever reason) that the intended follow up series which adds an implementer which actually requires this complex API, may never materialize/land, and in that case we are left with technical debt/an overcomplicated API for no reason.) > As for the need for the loop, let's not kid ourselves here: it was already > needed because some controllers have limited window sizes and do not allow > allocating large chunks of memory to map. That was never handled... I agree. For controllers which have a window size that is very small, the pci-epf-test driver will currently not handle buffer sizes larger than the window size. Adding a loop would solve that limitation. But this information is currently completely missing for the commit log. May I suggest that you: 1) Include a preparatory patch in this series, which adds the loop using the existing map functions. With the proper motivation in the commit log. 2) Add this patch which converts the pci-epf-test driver to use the new map API. The number of changed lines in this patch will thus be much smaller than it currently is, and it will be easier to review. > > I understand that certain EPF drivers will need to map buffers larger > > than that. But perhaps we can keep pci-epf-test.c simple, and introduce > > the more complex logic in EPF drivers that actually need it? > > But then it would not be much of test driver... We need to exercise corner cases > as well. Yes, your argument does make a lot of sense. > > More complicated EPF drivers can then choose if they want to support only > > the simple case (no looping case), but can also choose to support buffers > > larger than pci_epc_max_mapping_for_address(), using looping (I assume that > > each loop iteration will be able to map (at least) the same amount as the > > first loop iteration). > > How can they "choose" if the controller being used gives you a max mapping size > that completely depend on the PCI address used by the RC ? Unless the protocol > used by that function driver imposes constraint on the host on buffer alignment, > the epf cannot choose anything here. E.g. NVMe epf needs to be able to handle > anything. What I meant was that PCI EPF drivers could simply (continue) to return error if the buffer size was larger than the window size. But you have successfully changed my mind, let's just add the loop. (As my previous suggestion of letting an EPF driver call pci_epc_max_mapping_for_address()/pci_epc_map_align(), and then based on if the returned size being either smaller or larger than the window size, call either a function that does no looping, or a function that does looping, might actually be more error prone and more confusing compared to just having a single transfer function which includes a loop.) Kind regards, Niklas
