Re: [PATCH v6 3/5] PCI: endpoint: pci-epf-test: Add doorbell test support

[Niklas Cassel]

Hello Frank,

On Tue, Nov 12, 2024 at 12:48:16PM -0500, Frank Li wrote:
> Add three registers: doorbell_bar, doorbell_addr, and doorbell_data,
> along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a
> doorbell address space.
> 
> Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell
> callback handler by writing doorbell_data to the mapped doorbell_bar's
> address space.
> 
> Set doorbell_done in the doorbell callback to indicate completion.
> 
> To avoid broken compatibility, add new command COMMAND_ENABLE_DOORBELL
> and COMMAND_DISABLE_DOORBELL. Host side need send COMMAND_ENABLE_DOORBELL
> to map one bar's inbound address to MSI space. the command
> COMMAND_DISABLE_DOORBELL to recovery original inbound address mapping.
> 
> 	 	Host side new driver	Host side old driver
> 
> EP: new driver      S				F
> EP: old driver      F				F
> 
> S: If EP side support MSI, 'pcitest -B' return success.
>    If EP side doesn't support MSI, the same to 'F'.
> 
> F: 'pcitest -B' return failure, other case as usual.
> 
> Tested-by: Niklas Cassel <cassel@xxxxxxxxxx>
> Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> ---
> Change from v5 to v6
> - rename doorbell_addr to doorbell_offset

Is there a reason why you chose to not incorporate the helper function
that I suggested here:
https://lore.kernel.org/linux-pci/ZzMtKUFi30_o6SwL@ryzen/

I didn't see any reply from you to that message.

Personally I think that it is nice to have the alignment code in a single
function, rather than duplicating the code. The helper also looks quite
similar to how we do outbound address translation alignment:
https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=endpoint&id=e73ea1c2d4d8f7ba5daaf7aa51171f63cf79bcd8
so that people will recognize the pattern more easily.

But I guess you didn't like my suggestion?
(Which is fine, but I would have expected some motivation.)


Kind regards,
Niklas