Re: [PATCH] i2c: piix4: Continue probing for auxiliary SMBus without main

[Jean Delvare]

Daniel, 

Did you find the time to address my concerns? I think it would be the
right time to submit an updated patch if you want it to make it
upstream quickly.

Jean

On Mon, 28 Jul 2014 14:22:09 +0200, Jean Delvare wrote:
> Hi Daniel,
> 
> On Fri, 11 Jul 2014 20:06:15 -0400, Daniel M. Weeks wrote:
> > Some BIOS may only allow access to the AMD auxiliary SMBus - reserving
> > the main SMBus for system functions only. Probing should continue even
> > if the main bus is not available so at least the auxiliary can be added.
> > 
> > Signed-off-by: Daniel M. Weeks <dan@xxxxxxxxxxxx>
> > ---
> > This patch may warrant some discussion. I ran across this problem on an
> > AMD Hudson board where the main SMBus is hard-wired for debugging but
> > the auxiliary bus is exposed for expansion. Either purposefully or
> > because the BIOS is a little broken, loading this module used to result
> > in an ACPI conflict for the main bus and neither was usable. With the
> > patch I'm able to use the auxiliary bus regardless of the conflict on
> > the main bus. I'm not 100% convinced it's the correct fix - someone with
> > more ACPI experience than me is going to need to review this.
> 
> This is fine. The auxiliary bus setup code has its own ACPI resource
> conflict check, so it is safe to let the driver bind to the auxiliary
> bus even if the main bus is skipped.
> 
> > 
> > drivers/i2c/busses/i2c-piix4.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> > index a6f54ba..d4bac64 100644
> > --- a/drivers/i2c/busses/i2c-piix4.c
> > +++ b/drivers/i2c/busses/i2c-piix4.c
> > @@ -628,14 +628,16 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
> >  	else
> >  		retval = piix4_setup(dev, id);
> >  
> > -	/* If no main SMBus found, give up */
> > -	if (retval < 0)
> > -		return retval;
> > -
> > -	/* Try to register main SMBus adapter, give up if we can't */
> > -	retval = piix4_add_adapter(dev, retval, &piix4_main_adapter);
> > -	if (retval < 0)
> > -		return retval;
> > +	/* If no main SMBus found, do not give up
> > +	 * some BIOS purposely only expose aux bus */
> > +	if (retval < 0) {
> > +		dev_info(&dev->dev, "No main SMBus; checking auxiliary\n");
> 
> Not sure if this message is really needed, as both parts already print
> relevant message in both success and failure cases.
> 
> > +	} else {
> > +		/* Try to register main SMBus adapter, give up if we can't */
> > +		retval = piix4_add_adapter(dev, retval, &piix4_main_adapter);
> > +		if (retval < 0)
> > +			return retval;
> > +	}
> >  
> >  	/* Check for auxiliary SMBus on some AMD chipsets */
> >  	retval = -ENODEV;
> 
> I'm mostly fine with the change above, but it is insufficient in one
> corner case: if both the main bus and the aux bus probing fails. The
> code does currently not consider both buses equal, in that it never
> returns an error if the aux bus probing fails. The return value only
> stands for the status of the main bus. With your change, if both buses
> fail to register, the piix4_probe function may still returns 0
> (success.)
> 
> Your proposed change somehow makes the aux bus an equal of the main
> bus. The probe function should return 0 if either bus was successfully
> registered. It should still return an error if both fail to register.
> Unfortunately we can't report both error codes to the caller. For
> historical reasons I believe we should return the error code for the
> main bus in that case, which means you have to store it in the middle
> of the function for it to be available at the end of the function.
--
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/