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...
Cheers,
Michael
