Hi Finn,

Am 07.03.2018 um 13:54 schrieb Finn Thain:
> On Wed, 7 Mar 2018, Michael Schmitz wrote:
>> The major obstacle now seems to be dynamic allocation of the driver 
>> private data and storing a pointer to that in a way that it can be 
>> retrieved using just the esp pointer. dev_set_drvdata(esp->dev, zep) 
>> causes the module load to crash ...
> I've just noticed that most esp drivers do this:
> static int esp_foo_probe(struct platform_device *dev)
> {
>       ...
>       esp->dev = dev;
>       ...
> }
> But the esp->dev->dev dereferencing sometimes gets overlooked, resulting 
> in a pointer to a struct platform_device being used where a pointer to a 
> struct device should be used (i.e. dma_*() calls). I will look into fixing 
> this up. sun_esp.c doesn't have this problem, but the other drivers do.
> I don't think any of that applies to your zorro_esp code because the 
> version you sent does this,
>       esp->dev = &z->dev;
> which seems fine to me. But it could end up more convenient to use the 
> sun_esp approach and set esp->dev = z.

It just adds another dereferencing step in the dma_map functions which
we only need to do once here.

But sun_esp also does

        dev_set_drvdata(&op->dev, esp);

i.e. not only does it store the struct device pointer in the esp struct
(by indirection through the platform device struct), but also the struct
esp pointer in struct device.

> I suspect that the problem with zorro_esp is that sometimes you use the 
> esp->dev->driver_data pointer as the struct Scsi_Host pointer, and at 
> other times you use it for the struct zorro_driver_data pointer. (I think 
> I see now why you put the esp pointer in struct zorro_driver_data.)

The zorro_esp_priv pointer (zorro_esp_driver_data pointer are only used
to store board specific config data needed for probe), but yes, you've
found my error.

I overlooked that dev_set_drvdata() and zorro_set_drvdata() will store
their payload in the same struct device instance. Using one for struct
zorro_esp_priv and the other for struct Scsi_host is just asking for
trouble. Thanks for jogging my memory ...

Since there is no other place for me to put driver private data, I need
to change the use of that field to point to the current zorro_esp_priv
instance, and retrieve the struct esp pointer from there. I can retrieve
the host pointer from  struct esp so all should be well.

This is a little unusual so I better add a few comments to save the next
maintainer from unnecessary headache.

> If you like, email the current version to me or push it to a repo 
> somewhere and I'll take a look at it.

I'll take you up on that offer another time, but with the use of
dev->driver_data fixed, the driver no longer crashes. I shouldn't
hack kernel code in a rush...

Now on to mangle the rest of the issues raised in the review...



Reply via email to