> From: Leon Romanovsky <leon@xxxxxxxxxx>
> Sent: Friday, October 2, 2020 1:02 AM
> > > > > > +Registering an ancillary_device is a two-step process. First
> > > > > > +you must call ancillary_device_initialize(), which will check
> > > > > > +several aspects of the ancillary_device struct and perform a
> > > > > > +device_initialize(). After this step completes, any error
> > > > > > +state must have a call to put_device() in its
> > > > > resolution
> > > > > > +path. The second step in registering an ancillary_device is
> > > > > > +to perform a
> > > > > call
> > > > > > +to ancillary_device_add(), which will set the name of the
> > > > > > +device and add
> > > > > the
> > > > > > +device to the bus.
> > > > > > +
> > > > > > +To unregister an ancillary_device, just a call to
> > > > > ancillary_device_unregister()
> > > > > > +is used. This will perform both a device_del() and a put_device().
> > > > >
> > > > > Why did you chose ancillary_device_initialize() and not
> > > > > ancillary_device_register() to be paired with
> > > ancillary_device_unregister()?
> > > > >
> > > > > Thanks
> > > >
> > > > We originally had a single call to ancillary_device_register()
> > > > that paired with unregister, but there was an ask to separate the
> > > > register into an initialize and add to make the error condition
> > > > unwind more
> > > compartimentalized.
> > >
> > > It is correct thing to separate, but I would expect:
> > > ancillary_device_register()
> > > ancillary_device_add()
> > >
> > device_initialize(), device_add() and device_unregister() is the pattern widely
> followed in the core.
>
> It doesn't mean that I need to agree with that, right?
>
Right. May be I misunderstood your comment where you said "I expect device_register() and device_add()" in response to "device_initialize() and device_add".
I interpreted your comment as to replace initialize() with register().
Because that is odd naming and completely out of sync from the core APIs.
A helper like below that wraps initialize() and add() is buggy, because when register() returns an error, it doesn't know if should do kfree() or put_device().
ancillary_device_register(adev)
{
ret = ancillary_device_initialize();
if (ret)
return ret;
ret = ancillary_device_add();
if (ret)
put_device(dev);
return ret;
}
> >
> > > vs.
> > > ancillary_device_unregister()
> > >
> > > It is not a big deal, just curious.
> > >
> > > The much more big deal is that I'm required to create 1-to-1 mapping
> > > between device and driver, and I can't connect all my different
> > > modules to one xxx_core.pf.y device in N-to-1 mapping. "N"
> > > represents different protocols (IB, ETH, SCSI) and "1" is one PCI core.
> >
> > For one mlx5 (pf/vf/sf) device, there are three class erivers (ib, vdpa, en).
> >
> > So there should be one ida allocate per mlx5 device type.
> >
> > Static const mlx5_adev_names[MLX5_INTERFACE_PROTOCOL_MAX] = {
> > "rdma",
> > "eth",
> > "vdpa"
> > };
> >
> > Something like for current mlx5_register_device(),
>
> I know it and already implemented it, this is why I'm saying that it is not what I
> would expect from the implementation.
>
> It is wrong create mlx5_core.rdma.1 device that is equal to mlx5_core.eth.1 just
> to connect our mlx5_ib.ko to it, while documentation explains about creating
Ancillary bus's documentation? If so, it should be corrected.
Do you have specific text snippet to point to that should be fixed?
For purpose of matching service functionality, for each different class (ib, eth, vdpa) there is one ancillary device created.
What exactly is wrong here? (and why is it wrong now and not in previous version of the RFC?)
Do you want to create one device and 3 drivers to bind to it? If you think that way, may be pci core should be extended to support that, ancillary bus may not be required.
But that is different solution than what is being proposed here.
Not sure I understand your comment about wrong create mlx5_core.rdma1 equal to core.eth.1", because they are not equal.
To begin with, they have different id for matching service, and in future it should be extended to pass 'only' needed info.
mlx5_core exposing the 'whole' mlx5_core_dev to other drivers doesn't look correct.
> single PCI code device and other drivers connect to it.
>
> Thanks
>
>
> > mlx5_register_device()
> > {
> > id = ida_alloc(0, UINT_MAX, GFP_KERNEL);
> >
> > for (i = 0; I < MLX5_INTERFACE_PROTOCOL_MAX; i++) {
> > adev = kzalloc(sizeof(*adev), GFP_KERNEL);
> > adev.name = mlx5_adev_names[i];
> > adev.id = ret;
> > adev.dev.parent = mlx5_core_dev->device;
> > adev->coredev = mlx5_core_dev;
> > ret = ancillary_device_initialize(&adev);
> > ret = ancillary_device_register(adev); }
> >
> > This will create 3 ancillary devices for each PCI PF/VF/SF.
> > mlx5_core.rdma.1
> > mlx5_core.eth.1
> > mlx5_core.vdpa.1
> >
> > and mlx5_ib driver will do
> >
> > ancillary_driver_register()
> > {
> > For ID of mlx5_core.rdma.
> > }
> >
> > mlx5_vdpa driver does,
> >
> > ancillary_driver_register()
> > {
> > For ID of mlx5_core.vdpa
> > }
> >
> > This is uniform for pf/vf/sf for one or more all protocols.
