Re: [PATCH v3 02/30] drivers: hv: dxgkrnl: Driver initialization and loading

[Wei Liu]

On Wed, Mar 02, 2022 at 08:53:15AM +0100, Greg KH wrote:
> On Tue, Mar 01, 2022 at 10:23:21PM +0000, Wei Liu wrote:
> > > > +struct dxgglobal *dxgglobal;
> > > 
> > > No, make this per-device, NEVER have a single device for your driver.
> > > The Linux driver model makes it harder to do it this way than to do it
> > > correctly.  Do it correctly please and have no global structures like
> > > this.
> > > 
> > 
> > This may not be as big an issue as you thought. The device discovery is
> > still done via the normal VMBus probing routine. For all intents and
> > purposes the dxgglobal structure can be broken down into per device
> > fields and a global structure which contains the protocol versioning
> > information -- my understanding is there will always be a global
> > structure to hold information related to the backend, regardless of how
> > many devices there are.
> 
> Then that is wrong and needs to be fixed.  Drivers should almost never
> have any global data, that is not how Linux drivers work.  What happens
> when you get a second device in your system for this?  Major rework
> would have to happen and the code will break.  Handle that all now as it
> takes less work to make this per-device than it does to have a global
> variable.
> 

It is perhaps easier to draw parallel from an existing driver. I feel
like we're talking past each other.

Let's look at drivers/iommu/intel/iommu.c. There are a bunch of lists
like `static LIST_HEAD(dmar_rmrr_units)`. During the probing phase, new
units will be added to the list. I this the current code is following
this model. dxgglobal fulfills the role of a list.

Setting aside the question of whether it makes sense to keep a copy of
the per-VM state in each device instance, I can see the code be changed
to:

    struct mutex device_mutex; /* split out from dxgglobal */
    static LIST_HEAD(dxglist);
    
    /* Rename struct dxgglobal to struct dxgstate */
    struct dxgstate {
       struct list_head dxglist; /* link for dxglist */
       /* ... original fields sans device_mutex */
    }

    /*
     * Provide a bunch of helpers manipulate the list. Called in probe /
     * remove etc.
     */
    struct dxgstate *find_dxgstate(...);
    void remove_dxgstate(...);
    int add_dxgstate(...);

This model is well understood and used in tree. It is just that it
doesn't provide much value in doing this now since the list will only
contain one element. I hope that you're not saying we cannot even use a
per-module pointer to quickly get the data structure we want to use,
right?

Are you suggesting Iouri use dev_set_drvdata to stash the dxgstate
into the device object? I think that can be done too.

The code can be changed as:

    /* Rename struct dxgglobal to dxgstate and remove unneeded fields */
    struct dxgstate { ... };

    static int dxg_probe_vmbus(...) {

        /* probe successfully */

	struct dxgstate *state = kmalloc(...);
	/* Fill in dxgstate with information from backend */

	/* hdev->dev is the device object from the core driver framework */
	dev_set_drvdata(&hdev->dev, state);
    }

    static int dxg_remove_vmbus(...) {
        /* Normal stuff here ...*/

	struct dxgstate *state = dev_get_drvdata(...);
	dev_set_drvdata(..., NULL);
	kfree(state);
    }

    /* In all other functions */
    void do_things(...) {
        struct dxgstate *state = dev_get_drvdata(...);

	/* Use state in place of where dxgglobal was needed */

    }

Iouri, notice this doesn't change anything regarding how userspace is
designed. This is about how kernel organises its data.

I hope what I wrote above can bring our understanding closer.

Thanks,
Wei.