Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[Heikki Krogerus]

On Thu, May 28, 2015 at 08:36:46AM -0400, Sasha Levin wrote:
> On 05/28/2015 01:39 AM, Sudip Mukherjee wrote:
> > diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> > index 4eabfe2..1acae5b 100644
> > --- a/drivers/base/driver.c
> > +++ b/drivers/base/driver.c
> > @@ -150,6 +150,11 @@ int driver_register(struct device_driver *drv)
> >  	int ret;
> >  	struct device_driver *other;
> >  
> > +	if (!drv->bus->registered) {
> > +		pr_err("Driver %s registration failed. bus not yet registered\n",
> > +		       drv->name);
> > +		return -ENODEV;
> > +	}
> >  	BUG_ON(!drv->bus->p);
> 
> This is a design issue with the code in the layer above, there's no reason
> driver_register() should be called with a bus that wasn't registered to
> begin with.
> 
> This is why there's a BUG_ON there to catch these issues - it's a bug, not
> a desired behaviour.

Unfortunately problems with the design are not the only cases why we
could end up here before the bus has been registered. If the bus has
failed to register, we definitely should not trigger a BUG here. The
bus management driver has in that case already made the decision to
not BUG. Or if the user is allowed to disable a bus somehow, for
example with something like nousb parameter, but we still manage do
get here, we should again not trigger BUG().

I don't think BUG_ON here is ever the correct thing to do. This
function can see that the bus doesn't exist or possibly that something
has gone wrong by checking the "p", but it does not know any details,
nor should it. This function should trigger a warning in those cases
and return failure, and not make any extra decisions.


Thanks,

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/