From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752020AbeA2Xap (ORCPT ); Mon, 29 Jan 2018 18:30:45 -0500 Received: from mail-sn1nam02on0057.outbound.protection.outlook.com ([104.47.36.57]:15321 "EHLO NAM02-SN1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751529AbeA2Xam (ORCPT ); Mon, 29 Jan 2018 18:30:42 -0500 Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=andrew.cooks@opengear.com; Subject: Re: [2/3] i2c: piix4: fix number of ports on Family 16h Model 30h To: Guenter Roeck Cc: Jean Delvare , Wolfram Sang , "open list:I2C/SMBUS CONTROLLER DRIVERS FOR PC" , open list References: <93dea47498cbd29e9d37edd234122cd49252374c.1517195327.git.andrew.cooks@opengear.com> <20180129055824.GA15395@roeck-us.net> From: Andrew Cooks Message-ID: <7b07fce5-b7df-8ecc-6d51-34c0b5d8c458@opengear.com> Date: Tue, 30 Jan 2018 09:30:21 +1000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <20180129055824.GA15395@roeck-us.net> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [59.167.150.161] X-ClientProxiedBy: ME2PR01CA0019.ausprd01.prod.outlook.com (52.134.202.159) To CO2PR15MB0026.namprd15.prod.outlook.com (10.161.86.141) X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 1657b1f7-bf27-44df-9af9-08d567704f50 X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:(7020095)(4652020)(5600026)(4604075)(2017052603307)(7153060)(7193020);SRVR:CO2PR15MB0026; X-Microsoft-Exchange-Diagnostics: 1;CO2PR15MB0026;3:HyzHSIrqkt67IzQhSpB3vnTnwrkyMEUaw6/LCwLbH5YZ8EKrMADOpTA4qqfzUzpsiMGTI3J3oAiEFrU9tvOKfdDTPPcAQeShn+AeyUkRCGRmkjyBgoq61XYDHbtbp0cpSc5ypG/3Anjd4CjFAATjLDtyeyypSl4ZqMD2Ggn8xmZRaaOKDzUgHqVtCPcUw8eHdD/M2rcg7/744ED1liG7tGri/xPM9NnCg7/urfHuzBnlu8AzaEQtkYsv29U65ts0;25:kAeYWcAdQhl2qGqNBMlDQZamGdhRrfMdtngkIZ4SLOdm3Xy+/7xCdbEO89xiNHs80FM9WjLNP+/sENN4aQnFoWa/ke6ozmayAPysbRW2OY7iInqCy9w0cpRJTD8jX7RSGsLRzcqi9wbeoB0hhNJiOeH/FVWI5MFxfeAVEOc164fFWDT/FVMp9z6E2m8ebAPQCc2Y5Xlyyl7TeMMrLhQMx3E0MMftlQXEn/X1WsjoFxR8NfUX5l3XIkOMKS2t2VHOhZ3zOf399AgkBubykFY9nWKJZsvp0tUcNnWYOCFSlo6zsdUq6yq0jaYcuhbuDVQN7QJV0ZbxC4t+DS5cVK47F7a0RFktYJtDz7ylOEuOSIY=;31:pixIppm+27V2G2G/T3NW02g+YbSv/AMN9vyzGCHqvyRCyH/e4khTRqs1QpTJlx7JINVKBmxRaabCO1si4sYQWJq+yFGoSXbVKBMpCVZFy0R4KE8k0HUuH5zzYpqRy6kC89S3d6oA6NYFgoSrUsCFSnJXaLmKO/ZErQByO4XEFpI401uelSEQcm5snBsboWnil8iFzdabhaUjYNXgkYPlwzPIN5IX5rbrKxSnxloleNQ= X-MS-TrafficTypeDiagnostic: CO2PR15MB0026: X-Microsoft-Exchange-Diagnostics: 1;CO2PR15MB0026;20:KaxJKfTGkumlb1nRY+0aKOzkuZVDszkexHROdHUwWUBGnayRrHlxVo3QSnwdNcuYmDhHdprzKk2cTvQCMRmkZfNn4LRotE44PPN1e29hPGsxpPBtmm5tI9RMmwc7VpxKhnN1n4BxmiTMX8k5aAOpFyXZz5gKv2QiLyduIx1Ie0Q=;4:WlBuoo+cZAjIkzC86gXlygOn7wqyPr98jz+WjXE7tBOU84pFsRI9qYDVsslITT1XMHyJoX6/k2Krfo/rEHVq1QJVoAHJWuKrGcKmUG8ygOLYRRZzYLIChaiCzFtRCi17fj9PTW8YPTrf2GhgJf4EdPRXDBvi5Hm6ehBGmDyuKtc8wltMcyfZCeaLPpom9EX+7SDjroxTRJDJ5CN0qde738O29ichgxgJkPplKJjEBCOPBSF4GIUURWNfsRusfHRpR55EHMU3FMKvL9viCenUuA== X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(6040501)(2401047)(8121501046)(5005006)(3002001)(93006095)(93001095)(3231101)(944501161)(10201501046)(6041288)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123560045)(20161123564045)(20161123562045)(20161123558120)(6072148)(201708071742011);SRVR:CO2PR15MB0026;BCL:0;PCL:0;RULEID:;SRVR:CO2PR15MB0026; X-Forefront-PRVS: 0567A15835 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(6049001)(39840400004)(346002)(396003)(376002)(366004)(39380400002)(199004)(189003)(50466002)(23676004)(36756003)(76176011)(2486003)(16526019)(26005)(52116002)(52146003)(6486002)(316002)(86362001)(31686004)(230700001)(16576012)(117156002)(58126008)(4326008)(90366009)(478600001)(229853002)(81166006)(81156014)(305945005)(83506002)(7736002)(3846002)(25786009)(6116002)(68736007)(97736004)(64126003)(8936002)(66066001)(65956001)(65806001)(77096007)(106356001)(105586002)(47776003)(65826007)(6246003)(6916009)(5660300001)(386003)(2906002)(53546011)(31696002)(53936002)(8676002)(2950100002)(6666003)(54906003);DIR:OUT;SFP:1101;SCL:1;SRVR:CO2PR15MB0026;H:[192.168.254.67];FPR:;SPF:None;PTR:InfoNoRecords;A:1;MX:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtDTzJQUjE1TUIwMDI2OzIzOjRhV2FhQ3VRaEZYbmZ0QmpJVjZ0NE5zQVVR?= =?utf-8?B?R3ZTTkYrZHpyalR2WEVBRFIwQXJ6NnViMXpKVTdrUG9aQ3dTVHg1UDlkaHBy?= =?utf-8?B?WmVXRTRiMk5ROXV0emxYaFpNS0lvVVl2ZVd2T0NGQnk1ekNFdStXVnVuaEpT?= =?utf-8?B?a0dCVmhXcEd1NDVPZGl3NXZ0UVNTSkxzQjUxQXczTVgrV1ptWlN2SUZXd0I4?= =?utf-8?B?cHhxSytPN2I0NVpLdVVTQmZyaDJldzN3ZEpJOUFrWDNUUTJDNlh6Z3RNVHRX?= =?utf-8?B?ejJEL1VibURqUENqc29icFhybjZRdHYxaTY2d1ZtN3dEQzltWC9nTnZwYnhp?= =?utf-8?B?eUpqd0FTL2pOVHpQcG9UdDRjZzZuKzNJeS8vUk9BRG5CbDEzMXphVDRlZ2wy?= =?utf-8?B?TkhFblBWV3lKSzZPU0hVdjlzd0JkWWNURW1uZ0xwOTNuMGVja3BUSjRTTnZa?= =?utf-8?B?cXk3cU5VRlV1dFQ5WlNvcEtqYW1CbGw5OXBaaU5LK0dFaEVTZm0xQUxpUldj?= =?utf-8?B?RzR3RWJabmcxZ1BLbDFMbXhGVXNTMEMzYmh4RUwvazh0S2tFdnU1b3l0aGdY?= =?utf-8?B?QXNya2c0cEIyRmd6L0FGTG1Ta2dhc3p1TFQ5RzVBeUxwKzcvNDJLZVFDNTJi?= =?utf-8?B?Zk51R2svVWREUlNEOVUxclkxWE1FNjZORzY1VHlCdkNGVHd1Tjh3VDFZNVRF?= =?utf-8?B?UHBXVXh6cGZQRVV6MXVkMzBmOWdCUlo2SlArYVhoYlpHMytxaHlHd0ZDWWVR?= =?utf-8?B?ci9TMXRzS2FlbkFyTURGenB5c1JoeXJlS2w1ZDBTYnY5SHpETnhaU201QTd2?= =?utf-8?B?RnVxUkFCTHcwUFJLTk9ZUmVVeFQ0T0V1YUNYeGplZ2JoeEhGQVpaSlFUcWNy?= =?utf-8?B?OG82clpkSzBzL1h5b2p3dkM4dzNIank2MU5DbElES1FzbEhLTWF6SkZEUFJ0?= =?utf-8?B?b0o3bzNETDdyV2VwVWtRdk5iVHVzUmdwdXJZdXYrSDdOOUJwMkw3QXBTM1J5?= =?utf-8?B?dUV0M2tnTzZsbHM2Mmc1Rk9aZ1YwcEVGaUNDTmV3NEpuYVh0SVNLYUpXSXVz?= =?utf-8?B?RWxuYkhhVGtOQkNQcHNGdEFuZStrZjNBOGdVd3ZXczd5dVF3L3NBRWVxamto?= =?utf-8?B?WWZyckxIaVRkendwMW9SNXZ3cmVQRmp4VDc1THNBaXVHZjUyTENlTHBVOWtX?= =?utf-8?B?QXV4ZU1KREtESFdRQTZXcEN0M3V2RkJVZ2NUeFkvdjV0Skh5VGhVTytwQk4w?= =?utf-8?B?TGx2TjZza0FEekZiVllLQ0U4RjFNbHlmK2ZDbkxFMEtEU0FtSnpMZ1p5M0F3?= =?utf-8?B?d2xQVFBxRjhpWkcvTEFVblJFMFRRUHE3Vm9HV3NwYkFzVlpyN052TjFVRU1F?= =?utf-8?B?WU9HSmRhdjEzSUV2Y0tkNXI2b1ZIMUVxSXpRbUVaaU5XcFN6QzB1VGx3a0RC?= =?utf-8?B?aHpNWS96K0FpOGlhV0RKN2dHWjk5QmlZK3dUT3hCL3haWUJhOHd0WDdKUkEx?= =?utf-8?B?aC9OMWlVTy9KdE1PUU9IaS8xNGFndHVpTnh3MmVhejVabndqK3dkYmNva3Vp?= =?utf-8?B?OXZtSytzZ1ZhVHFXemhOMytaclhRQW5DOWpYOFlMdVNSbFQ1aXRXZTI3ZmRn?= =?utf-8?B?N2cwOUZPYm8rU0JCSmtDOGZIdXN4VHVITTVrdnlJNHdpNmxpeWFSV0JvNXBk?= =?utf-8?B?Sm5WUG1jR2NCQjljRlZFdm8xclY5aE9wZGhCazlFZ1luekJVTjBtVjlaMytG?= =?utf-8?B?QkF6dG9DTGhiQUxIdFFqbzlJUjltK0RyYjRacEl4eUZpbXNvVHBhOU9HSlUy?= =?utf-8?B?QXR0dmltRlp2M2JsQSttTDdBaGlNTi8xTEsxK1BsTWd1YXhCZ2hSN2pZejhT?= =?utf-8?Q?k4Ab3SGoXkCjgCJyA9ZtiEOlhnpTgGe0?= X-Microsoft-Exchange-Diagnostics: 1;CO2PR15MB0026;6:Zos+/mwH/LN+kJNtKlmYNlc5YAtgGwRGdPeLMfk7rwruQFHTFcOjFIyuMs0YF2ip5zSQH+mKeR9odebEKcXP62+Vf7PILrOaFp2bX+QouXyl9CQi2nH8H1P41gIhDCwHA1WiaWZk61nIAEM8+bN7M6FBpo7Se6chSEFYeIkHluBogVcVUjeOqbvX/djIyIWmCcqNFnIdaPU6nXCmiItOClhGnyr7zUOi2bYjg8euBwAhHtl0ji/ciUdIq7J7plDRQEvloJXe6P7CQaZ+YpZTglWUYf/UzCaU/Ou0PiiMbPCUtFIyvYteH83W8ehxqQJ4MpEPfXI3OiVZ83p+7msW7wT8iwUzLcv4XFCLh1Dehrs=;5:70Go/ZsuRkOud8evGdFxj0gh82CrRtQf5EqOg59X+n7DU3bP524u6opZ+WjTQXFVBvCRHeaccf/L/FsyqESNVzBZWwaNzzqaevCH5YUpFl5yygqqCHd+vB3eYO0Ozryh64x8PxSxD2UdjsJptRWU4gha2Fda8/+NiB4bu+ITEZg=;24:LnEpm1jDkbT/uAXwRMtvbFHH9KLTBRcUZMKkKSmGnP8C77gzLBduKIlFfEJ07X4xrkZS7Sq6h92n9tgcxImj4L+UV47wrRr99A/AgMaRglg=;7:djag/Ez9aKXtq3EK7gkgSvgz4gM+NWfTa6juC/AcalwYyqQI/KPSqC7E02sY1o0a26yTWYxosGCx09Kj15hM2AFRjCVDBjojPRnWFs5sNPNHEM/MMT5I0EQDgj1CfpadB7u34I1zmprgCCME3LCNtMyX/bFp+RpTmQXEixBp6i9ea6+qilRs2Z9o+PGBo6Fn+dCe7KaD8Ud1+oioKuRciFtyGVh1AtpDRHbcHzUs/41o9KE6XWyeaBWgcNG/WIKw SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: opengear.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Jan 2018 23:30:38.4035 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: 1657b1f7-bf27-44df-9af9-08d567704f50 X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: a6251c26-d21f-4164-a225-1f4eaebf5f9a X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO2PR15MB0026 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> --- >> 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.