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
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
