Re: [PATCH v4] NTB: Add IDT 89HPESxNTx PCIe-switches support

[Logan Gunthorpe <lsgunthorpe@gmail.com>]

On 07/03/17 08:09 AM, Serge Semin wrote:
> No I shouldn't. The thing is, that NTB API has two levels of drivers: client
> and hardware drivers. IDT NTB driver you are reviewing is hardware driver, it
> refers to the actual PCIe-device and is responsible for communications with it.
> The client drivers use NTB API abstraction and common facilities it exposes to
> implement some user-available functionality. They communicate with NTB devices
> registered by the hardware drivers. So to speak the client drivers work with
> ndev->ntb.dev, but the hardware drivers - with ndev->ntb.pdev->dev. That's why
> it is necessary to separate the logs of these two levels.

Huh? Besides the weirdness in the NTB tree, show me one driver that
prints to its
pci device. You created ntb.dev and it's a member of idt_ntb_dev, the
pdev is a pointer
somewhere else which should be a huge hint.  Also, see [1] for someone's
opinion with a little more clout than mine. The other drivers in the NTB
tree are meant to be changing in this regard too, if they haven't already.

>>> It's obvious, to prevent a race condition of access to the Mapping table, which
>>> is implemented by Address and Data registers set.
>>> For the same reason the driver have spin lock to protect an access to the
>>> Global Switch registers using GASA-ADDR and GASA-DATA.
>> Ah, I see. Perhaps these should be in their own function in the same way
>> as the gasa functions? It would probably be a bit clearer.
>>
> I would rather leave it as is. It's just two lines of code, which are present
> just at one place of the driver. 
By my count, there are two writes and two reads but that's hardly the
point. Even if there is only one, a function is a good way to add
documentation and make the calling function look a little cleaner.

> It's not NTB, but in general devres isn't that wide spread in the kernel drivers,
> since the technology is pretty new. Most of drivers developers are just used to
> do allocations/enables and deallocations/disables/releases manually in the code.
> Although it's really handy to get being familiar with devres methods.

Most drivers that use a struct device typically will use the reference
counting on the device with it's release function -- not devm_kzalloc.
This makes reference counting work correctly as a device often has to
persist after it's parent device goes away. See [2][3][4] which are all
fairly modern examples (and I could give you more). I'm not asking you
to change it though as this is because of how NTB decided to do things.

Logan

[1] https://lkml.org/lkml/2017/1/11/26
[2] drivers/char/tpm/tpm-chip.c:150
[3] drivers/input/mousedev.c:854
[4] drivers/rapidio/devices/rio_mport_cdev.c:2438