Hi Guenter
Thanks for your review!
On 29/01/18 15:58, Guenter Roeck wrote:
> On Mon, Jan 29, 2018 at 01:54:19PM +1000, Andrew Cooks wrote:
>> Prevent bus timeouts and resets on Family 16h Model 30h), by not
>> probing reserved Ports 3 and 4.
>>
>> According to the AMD BIOS and Kernel Developer's Guides (BKDG), Port 3
>> and Port 4 are reserved on the following devices:
>> - Family 15h Model 60h-6Fh,
>> - Family 15h Model 70h-7Fh,
>> - Family 16h Models 30h-3Fh,
>>
>> Signed-off-by: Andrew Cooks <andrew.cooks@xxxxxxxxxxxx>
>> ---
>> drivers/i2c/busses/i2c-piix4.c | 31 ++++++++++++++++++++++---------
>> 1 file changed, 22 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
>> index 89692f4..9763241 100644
>> --- a/drivers/i2c/busses/i2c-piix4.c
>> +++ b/drivers/i2c/busses/i2c-piix4.c
>> @@ -82,6 +82,13 @@
>> /* Multi-port constants */
>> #define PIIX4_MAX_ADAPTERS 4
>>
>> +/*
>> + * Main adapter port count. At least one (Port 0) plus up to 3 additional
>> + * (Ports 2-4)
>> + */
>
> This is a bit misleading. Which adapter has only one port ?
The main adapter has at least one port (Port 0), but may have up to four in total, depending on the chip version. Ports are labeled 0, 2, 3, 4 (with 1 being reserved).
The aux adapter has only one port and is labeled "port 1" in this driver, but not in the AMD documentation.
If the comment isn't helpful, I think I'll just remove it.
>
>> +#define SB800_MAIN_PORTS 4
>> +#define HUDSON2_MAIN_PORTS 2 /* HUDSON2, reserves Port 3 and Port 4 */
>> +
> A tab between define and value might be helpful.
will fix.
> Not sure though why both PIIX4_MAX_ADAPTERS and SB800_MAIN_PORTS
> are needed.
PIIX4_MAX_ADAPTERS keeps the max array length.
SB800_MAIN_PORTS maintains the existing behavior for the many chips previous chips. It would be a big job to audit all of them.
Their meaning is slightly different, but I think I'll just drop SB800_MAIN_PORTS and reuse PIIX4_MAX_ADAPTERS. I guess more invasive refactoring to remove the fixed size array is also possible.
>
>> /* SB800 constants */
>> #define SB800_PIIX4_SMB_IDX 0xcd6
>>
>> @@ -800,6 +807,7 @@ MODULE_DEVICE_TABLE (pci, piix4_ids);
>>
>> static struct i2c_adapter *piix4_main_adapters[PIIX4_MAX_ADAPTERS];
>> static struct i2c_adapter *piix4_aux_adapter;
>> +static int piix4_adapter_count;
>
> This variable is never set, only decreased.
Thanks, will fix.
>
>>
>> static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
>> bool sb800_main, u8 port, bool notify_imc,
>> @@ -856,10 +864,17 @@ static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba,
>> bool notify_imc)
>> {
>> struct i2c_piix4_adapdata *adapdata;
>> - int port;
>> + int port, port_count;
>> int retval;
>>
>> - for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) {
>> + if (dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS ||
>> + dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) {
>> + port_count = HUDSON2_MAIN_PORTS;
>> + } else {
>> + port_count = SB800_MAIN_PORTS;
>> + }
>> +
>> + for (port = 0; port < port_count; port++) {
>> retval = piix4_add_adapter(dev, smba, true, port, notify_imc,
>> piix4_main_port_names_sb800[port],
>> &piix4_main_adapters[port]);
>> @@ -872,7 +887,7 @@ static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba,
>> error:
>> dev_err(&dev->dev,
>> "Error setting up SB800 adapters. Unregistering!\n");
>> - while (--port >= 0) {
>> + while (--piix4_adapter_count >= 0) {
>
> This is wrong, even if piix4_adapter_count was initialized.
Yes, how careless of me. Will fix.
>
>> adapdata = i2c_get_adapdata(piix4_main_adapters[port]);
>> if (adapdata->smba) {
>> i2c_del_adapter(piix4_main_adapters[port]);
>> @@ -993,12 +1008,10 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
>>
>> static void piix4_remove(struct pci_dev *dev)
>> {
>> - int port = PIIX4_MAX_ADAPTERS;
>
> A one line change would do it just as well.
> int port = piix4_adapter_count;
Will fix.
>
>> -
>> - while (--port >= 0) {
>> - if (piix4_main_adapters[port]) {
>> - piix4_adap_remove(piix4_main_adapters[port]);
>> - piix4_main_adapters[port] = NULL;
>> + while (--piix4_adapter_count >= 0) {
>> + if (piix4_main_adapters[piix4_adapter_count]) {
>> + piix4_adap_remove(piix4_main_adapters[piix4_adapter_count]);
>> + piix4_main_adapters[piix4_adapter_count] = NULL;
>> }
>> }
>>
Thanks again for the review and feedback!
a.
