On Sat, Sep 21, 2024 at 01:00:52PM +0000, Winkler, Tomas wrote:
> > On Thu, Sep 19, 2024 at 09:54:24AM +0000, Winkler, Tomas wrote:
> > > > On Mon, Sep 16, 2024 at 04:49:17PM +0300, Alexander Usyskin wrote:
> > Just do normal open coded allocations, the reference counting is just
> > obscure.
> The kref here is for reason so we don't need to hunt the close open, I frankly don't understand
> what is wrong with it,
It's locking/refcounting stuff that looks nothing like any other SPI
controller driver. Even if it works it's obviously fragile since the
driver does surprising things which break assumptions that will be made
by people not looking at this specific driver, and causes people to have
to spend more effort figuring out what you're doing. If there is any
benefit to doing this then open coding it in one specific driver is
clearly not the right place to do it.
> > > > > +static void intel_dg_spi_remove(struct auxiliary_device *aux_dev) {
> > > > > + struct intel_dg_spi *spi = dev_get_drvdata(&aux_dev->dev);
> > > > > + if (!spi)
> > > > > + return;
> > > > > + dev_set_drvdata(&aux_dev->dev, NULL);
> > > > If the above is doing anything there's a problem...
> > o
> > > It makes sure the data is set to NULL.
> > Which is needed because...?
> This is a boilerplate part, the content is consequent patches.
Which would come back to the issues created by the random splitting of
the series were it not for the fact that if anything tries to look at
the driver data of a removed device it's buggy, the reference is gone
and the device may have been deallocated and it's certainly freed from
the perspective of this user. Notice how other drivers don't do this.
The driver core will also overwrite the driver data of released
devices...
At a high level a lot of the issues with this series is that both in
terms of how it's been sent and what it's doing there's a bunch of
things that look nothing like how we normally handle things. At best
this means that problems are being solved at the wrong level, but it's
hard to see that this is the case.
Attachment:
signature.asc
Description: PGP signature
