On Tue, Jun 08, 2021 at 09:30:23AM -0300, Jason Gunthorpe wrote:
> On Tue, Jun 08, 2021 at 08:47:19AM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Jun 07, 2021 at 09:55:45PM -0300, Jason Gunthorpe wrote:
> > > Currently really_probe() returns 1 on success and 0 if the probe() call
> > > fails. This return code arrangement is designed to be useful for
> > > __device_attach_driver() which is walking the device list and trying every
> > > driver. 0 means to keep trying.
> > >
> > > However, it is not useful for the other places that call through to
> > > really_probe() that do actually want to see the probe() return code.
> > >
> > > For instance bind_store() would be better to return the actual error code
> > > from the driver's probe method, not discarding it and returning -ENODEV.
> >
> > Why does that matter? Why does it need to know this?
>
> Proper return code to userspace are important. Knowing why the driver
> probe() fails is certainly helpful for debugging. Is there are reason
> to hide them? I think this is an improvement for sysfs bind.
>
> Why this series needs it is because mdev has fixed sys uAPI at this point
> that requires carring the return code from device driver probe() to
> a mdev sysfs function.
What is mdev and what userspace tool requires such a userspace api to
depend on this?
Tools doing manual bind/unbind from userspace are crazy, it's always
been a "look at this neat hack!" type of thing. To do it "right" you
should always do it correctly within the kernel.
> > > +enum {
> > > + /* Set on output if the -ERR has come from a probe() function */
> > > + PROBEF_DRV_FAILED = 1 << 0,
> > > +};
> > > +
> > > +static int really_probe(struct device *dev, struct device_driver *drv,
> > > + unsigned int *flags)
> >
> > Ugh, no, please no functions with random "flags" in them, that way lies
> > madness and unmaintainable code for decades to come.
>
> The alternative to this something like this:
>
> static int really_probe(struct device *dev, struct device_driver *drv,
> int *probe_err)
>
> And since we still need the 'do not probe defer' in next patches then
> it would have to be this:
>
> static int really_probe(struct device *dev, struct device_driver *drv,
> int *probe_err, bool allow_probe_defer)
>
> And the two new arguments flowed up through several function call
> sites.
>
> Do you prefer one of these more?
Random boolean flags as parameters are just as bad.
Make the functions able to be understood when read.
> For your other question PROBEF_ means 'probe flag'.
That was not obvious at all, and not something I would remember the next
time I have to look at this code...
Please use full words, we don't have a limit on restricted characters
anymore, this isn't the 1980's...
thanks,
greg k-h
