Date:   Thu, 2 Feb 2023 20:47:30 +0100
From:   Henning Schild <henning.schild@siemens.com>
To:     Pavel Machek <pavel@ucw.cz>, Lee Jones <lee@kernel.org>,
        Hans de Goede <hdegoede@redhat.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        <linux-leds@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] leds: simatic-ipc-leds-gpio: do not run into endless
 EPROBE_DEFER loop
Message-ID: <20230202204730.7ee24c01@md1za8fc.ad001.siemens.net>
In-Reply-To: <20230125181711.30844-1-henning.schild@siemens.com>
References: <20230125181711.30844-1-henning.schild@siemens.com>
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?xOkFJY5Omylxfz+70avdPM9RGnFEXJcQzWYprvw0H0BX1QYM0YtGJXsMVX/K?=
 =?us-ascii?Q?spTRKV302NV5MoqDxcCFe3MoGEZVf2lvtNRDY/Idxdp1/HLnVwJFsrMH/Vep?=
 =?us-ascii?Q?IGa7adMIqBRC2T7H5TzVhwuCQ8FjcH17Pn9wZeWtHcfGkT3ThA97NDSbUgfF?=
 =?us-ascii?Q?sg8YXpgDP3HyH05G/Kv5pEKUXUmIdFDWeJ1jPhMxBZwgyY2bQL5Ek0hvfUze?=
 =?us-ascii?Q?sVpvRBVaOvBFN2SbVHkbmAxzpn10rikkkDWQFkEaSeekE/8utU2ptH4MOazQ?=
 =?us-ascii?Q?i0HsMt1xCREnl0smBrifvbPl378hgxSdDyBEn95YEmkYamZcLLZ9ZdPXe1Hs?=
 =?us-ascii?Q?5c/QrKdEaauA0ZVTJzR+lfxNLxmbVPoIGu5iFZ+Kk9/9WRAxmpsd8nL8NZBf?=
 =?us-ascii?Q?CtCHSCXNavYSLva8f7s6giZ89HmbxdxXaWdmSv/BtYfY8ImA2Goo4biNMw6c?=
 =?us-ascii?Q?1FRKCGKnPQOau5C7wrkYUt/RAShlkxtcv8jwfm/7V0Ctnl5s8D4vuYqFS2p3?=
 =?us-ascii?Q?snjFZd/c9IqIsCTDuxAbUCfPurnTAkU9Gqst/8vz1nC/kOSoQtK//xXQwlKD?=
 =?us-ascii?Q?p98OqR8vdvMpEs5WOONPTZ11mnjfHZ2bOOqjkGLGSyUirTWIgQzUe7IN+il7?=
 =?us-ascii?Q?SQR3QAQDKK3oP9lBETRu+H/pmdrYllt/JpUDNE1Xcr5C5LjXc2UmF+Zlwty8?=
 =?us-ascii?Q?IEYF1grQV5niUFfEBaUCBqA8u6mS/7kMwY8k2oNhCZ3GBNAMu+yG0UDimKZl?=
 =?us-ascii?Q?+PHm4X3qrW2pdXcVuVAI16nc0iMYSDQkAXpOVAnDUgiZt+J6uceqHXwX3+8H?=
 =?us-ascii?Q?qsjGfxK4zf7+NqcVwLJTdigqi/tj0YerFR2jOL3jRisal7ax5AtMp5ucgxXG?=
 =?us-ascii?Q?WTCd3TDnrSQ5vOVx/f1IrwxEbXah8NpaM9xnh2Ki7Z8anMTnnYUstZbL7Cqo?=
 =?us-ascii?Q?YulHGsFb1/khk2ZatZkPSila20/X9UqxZjGBB6EUBZAWFidK5IezBnL/p/iM?=
 =?us-ascii?Q?9FUr5gfBjDLjgMZ+6980FScmJrSnUG2QqQc+tBeTi1zbHsJmHjFVHnCiDVAC?=
 =?us-ascii?Q?05+qoUYtsQ2nRSUHhOHopqYRFxjWVZKDurLAN6vkk8DPhp1TWvELoAc1YhTi?=
 =?us-ascii?Q?Dox+q344fWervNeCdd+IsR/IE4zfX3d62XX/TzFxADPqyvHygGMxx1LobJaY?=
 =?us-ascii?Q?nnfFQSkJYY+BAOwYQbmqz6pM0qHHx9YUMsYmFjqPBNAGZKh+i36itwgTZXcw?=
 =?us-ascii?Q?8tA0BudR7yu0rRa/FPFWbsTRTJ+ulFbQANyR/bx2IomRr0eyB0i3qj6fELjy?=
 =?us-ascii?Q?6KWsrwaBONfsWQieJUowZThhffHOrGWJehbxsWLJbZ3QeFDvzKFCpvRa+FD5?=
 =?us-ascii?Q?9Fjq1X46Mb1jMKsbzzXrKWFIJM6U9KrpzXzBmgZK1HG7WLSwXUiHuNluympf?=
 =?us-ascii?Q?fH8+1HYgMHCoLgQ5QIKpuRIJ5qU7CiMRpkUwcL2Asiv1ANGoxRudywxjOvbK?=
 =?us-ascii?Q?DAzKx76OwUkGxI2OeYMxfJ0DKLvhtsUpxIO3lnta9q3/b3biywlqJGhq2Vig?=
 =?us-ascii?Q?Rtr0weME5fmgqI9Uh98665OF3c2jKiU+YhBKhhwcejulN2BBRQVJ5iHYefG+?=
 =?us-ascii?Q?ZifWWHfpUx5R97niSRX3xGe3MctejFdx3regPu3u8woMkCm6DYsGLDnZdUSR?=
 =?us-ascii?Q?RIF3DQ=3D=3D?=
X-OriginatorOrg: siemens.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 64411481-3fc7-42d0-6c69-08db05565460
X-MS-Exchange-CrossTenant-AuthSource: PA4PR10MB5780.EURPRD10.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Feb 2023 19:47:34.7594
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 38ae3bcd-9579-4fd4-adda-b42e1495d55a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: DY1slQge68+RCbYbi9RuwQY/2cy1zGuIk5Q6fou9ZrBji63H/RSOFKNq/Yp7d1ZQ9+AarMOgkkCBcM6zW6W0r9andHdcSaBTJckS4oWMRcc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PR3PR10MB4078
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Am Wed, 25 Jan 2023 19:17:11 +0100
schrieb Henning Schild <henning.schild@siemens.com>:

> Should the driver providing our GPIOs not be available we used to
> return -EPORBE_DEFER out of the probe function and cause a
> potentially endless loop that also printed a lot to the kernel log.
> 
> ...
> leds-gpio leds-gpio: cannot find GPIO chip igpio-f7188x-2, deferring
> leds-gpio leds-gpio: Skipping unavailable LED gpio 0 (red:status-1)
> ...
> 
> The "leds-gpio" just ignores all entries and would never try again
> even if the GPIOs show up later. But our extra two GPIOs could cause
> that loop, in which we would even register/unregister "leds-gpio" and
> cause all the printing.
> 
> If any of those two extra GPIOs is not there, return with -ENODEV
> instead of -EPROBE_DEFER.

This is a really bad idea. The real fix for the future will be to
write individual drivers which clearly describe their dep chains.

This patch should not be merged.

Henning
 
> Fixes: a6c80bec3c93 ("leds: simatic-ipc-leds-gpio: Add GPIO version
> of Siemens driver") Signed-off-by: Henning Schild
> <henning.schild@siemens.com> ---
>  drivers/leds/simple/simatic-ipc-leds-gpio.c | 29
> +++++++++------------ 1 file changed, 13 insertions(+), 16
> deletions(-)
> 
> diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio.c
> b/drivers/leds/simple/simatic-ipc-leds-gpio.c index
> 07f0d79d604d..4e2595e684c6 100644 ---
> a/drivers/leds/simple/simatic-ipc-leds-gpio.c +++
> b/drivers/leds/simple/simatic-ipc-leds-gpio.c @@ -74,14 +74,13 @@
> static int simatic_ipc_leds_gpio_probe(struct platform_device *pdev)
> const struct simatic_ipc_platform *plat = pdev->dev.platform_data;
> struct gpio_desc *gpiod; int err;
> +	int i;
>  
>  	switch (plat->devmode) {
>  	case SIMATIC_IPC_DEVICE_127E:
>  		simatic_ipc_led_gpio_table =
> &simatic_ipc_led_gpio_table_127e; break;
>  	case SIMATIC_IPC_DEVICE_227G:
> -		if (!IS_ENABLED(CONFIG_GPIO_F7188X))
> -			return -ENODEV;
>  		request_module("gpio-f7188x");
>  		simatic_ipc_led_gpio_table =
> &simatic_ipc_led_gpio_table_227g; break;
> @@ -99,21 +98,19 @@ static int simatic_ipc_leds_gpio_probe(struct
> platform_device *pdev) goto out;
>  	}
>  
> -	/* PM_BIOS_BOOT_N */
> -	gpiod = gpiod_get_index(&simatic_leds_pdev->dev, NULL, 6,
> GPIOD_OUT_LOW);
> -	if (IS_ERR(gpiod)) {
> -		err = PTR_ERR(gpiod);
> -		goto out;
> -	}
> -	gpiod_put(gpiod);
> -
> -	/* PM_WDT_OUT */
> -	gpiod = gpiod_get_index(&simatic_leds_pdev->dev, NULL, 7,
> GPIOD_OUT_LOW);
> -	if (IS_ERR(gpiod)) {
> -		err = PTR_ERR(gpiod);
> -		goto out;
> +	for (i = 0; i < 2; i++) {
> +		gpiod = gpiod_get_index(&simatic_leds_pdev->dev,
> NULL,
> +
> ARRAY_SIZE(simatic_ipc_gpio_leds) + i, GPIOD_OUT_LOW);
> +		if (IS_ERR(gpiod)) {
> +			err = PTR_ERR(gpiod);
> +			if (err == -EPROBE_DEFER) {
> +				dev_err(&pdev->dev, "GPIO driver
> seems missing\n");
> +				err = -ENODEV;
> +			}
> +			goto out;
> +		}
> +		gpiod_put(gpiod);
>  	}
> -	gpiod_put(gpiod);
>  
>  	return 0;
>  out: