Message-ID: <62bbfb0e-20bd-47f7-ada1-7f4d30c888d7@kernel.org>
Date: Mon, 26 Jan 2026 15:34:25 +0900
From: Damien Le Moal <dlemoal@...nel.org>
To: Markus Probst <markus.probst@...teo.de>, Lee Jones <lee@...nel.org>,
 Pavel Machek <pavel@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Jacek Anaszewski <jacek.anaszewski@...il.com>,
 Niklas Cassel <cassel@...nel.org>, John Garry <john.g.garry@...cle.com>,
 Jason Yan <yanaijie@...wei.com>,
 "James E.J. Bottomley" <James.Bottomley@...senPartnership.com>,
 "Martin K. Petersen" <martin.petersen@...cle.com>
Cc: Pavel Machek <pavel@....cz>, linux-leds@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-ide@...r.kernel.org, linux-scsi@...r.kernel.org
Subject: Re: [PATCH RFC 4/4] leds: add disk trigger for each ata port

On 1/24/26 4:05 AM, Markus Probst wrote:
> Register a disk trigger for each ata port. This trigger will only show
> the activity for the ata port it has been registered for.
> 
> This allows individual leds to be mapped to one ata port.
> This is especially useful for NAS devices, which have an own led for each
> disk slot.
> 
> Signed-off-by: Markus Probst <markus.probst@...teo.de>
> ---
>  drivers/ata/libata-core.c           |  22 +++++-
>  drivers/leds/trigger/ledtrig-disk.c | 144 ++++++++++++++++++++++++++++++------
>  drivers/scsi/libsas/sas_ata.c       |   3 +-
>  include/linux/leds.h                |  16 +++-
>  include/linux/libata.h              |   6 +-
>  5 files changed, 161 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 09d8c035fcdf..796c46449298 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4921,8 +4921,10 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
>  	struct ata_device *dev = qc->dev;
>  	struct ata_eh_info *ehi = &dev->link->eh_info;
>  
> +#ifdef CONFIG_LEDS_TRIGGER_DISK
>  	/* Trigger the LED (if available) */
> -	ledtrig_disk_activity(!!(qc->tf.flags & ATA_TFLAG_WRITE));
> +	ledtrig_disk_activity(ap->led_trigger, !!(qc->tf.flags & ATA_TFLAG_WRITE));
> +#endif

Please define an empty wrapper for the !CONFIG_LEDS_TRIGGER_DISK case to avoid
adding this ifdef.

>  
>  	/*
>  	 * In order to synchronize EH with the regular execution path, a qc that
> @@ -5538,10 +5540,13 @@ int sata_link_init_spd(struct ata_link *link)
>   *	LOCKING:
>   *	Inherited from calling layer (may sleep).
>   */
> -struct ata_port *ata_port_alloc(struct ata_host *host)
> +struct ata_port *ata_port_alloc(struct ata_host *host, int port_no)
>  {
>  	struct ata_port *ap;
>  	int id;
> +#ifdef CONFIG_LEDS_TRIGGER_DISK
> +	char name[32];
> +#endif
>  
>  	ap = kzalloc(sizeof(*ap), GFP_KERNEL);
>  	if (!ap)
> @@ -5557,6 +5562,7 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
>  	ap->print_id = id;
>  	ap->host = host;
>  	ap->dev = host->dev;
> +	ap->port_no = port_no;

Please extract this change in a prep patch.

>  
>  	mutex_init(&ap->scsi_scan_mutex);
>  	INIT_DELAYED_WORK(&ap->hotplug_task, ata_scsi_hotplug);
> @@ -5579,6 +5585,11 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
>  
>  	ata_force_pflags(ap);
>  
> +#ifdef CONFIG_LEDS_TRIGGER_DISK
> +	if (snprintf(name, sizeof(name), "%s-ata%d", dev_name(host->dev), port_no) < sizeof(name))
> +		ap->led_trigger = ledtrig_disk_trigger_register(name);
> +#endif

Same here: please define a helper function which is void for the
!CONFIG_LEDS_TRIGGER_DISK case. That will avoid both ifdefs here. Sprinkling
the code with such ifdef makes maintenance a nightmare. Wrap everything to
avoid that please.

> +
>  	return ap;
>  }
>  EXPORT_SYMBOL_GPL(ata_port_alloc);
> @@ -5588,6 +5599,10 @@ void ata_port_free(struct ata_port *ap)
>  	if (!ap)
>  		return;
>  
> +#ifdef CONFIG_LEDS_TRIGGER_DISK
> +	ledtrig_disk_trigger_unregister(ap->led_trigger);
> +#endif

Same comment. Use a void function or null macro to define this for the
!CONFIG_LEDS_TRIGGER_DISK case.

> +
>  	kfree(ap->pmp_link);
>  	kfree(ap->slave_link);
>  	ida_free(&ata_ida, ap->print_id);
> @@ -5690,11 +5705,10 @@ struct ata_host *ata_host_alloc(struct device *dev, int n_ports)
>  	for (i = 0; i < n_ports; i++) {
>  		struct ata_port *ap;
>  
> -		ap = ata_port_alloc(host);
> +		ap = ata_port_alloc(host, i);
>  		if (!ap)
>  			goto err_out;
>  
> -		ap->port_no = i;
>  		host->ports[i] = ap;
>  	}
>  
> diff --git a/drivers/leds/trigger/ledtrig-disk.c b/drivers/leds/trigger/ledtrig-disk.c
> index ed5ef83a5b35..fd25b4e45fb4 100644
> --- a/drivers/leds/trigger/ledtrig-disk.c
> +++ b/drivers/leds/trigger/ledtrig-disk.c

Can you try to split this part into a prep patch to avoid mixing led code and
ata code ?

> @@ -159,20 +159,22 @@ static int ledtrig_disk_activate(struct led_classdev *led_cdev)
>  	return 0;
>  }
>  
> -static struct led_trigger ledtrig_disk = {
> -	.name = "disk-activity",
> -	.activate = ledtrig_disk_activate,
> -	.groups = ledtrig_disk_groups,
> -};
> -static struct led_trigger ledtrig_disk_read = {
> -	.name = "disk-read",
> -	.activate = ledtrig_disk_activate,
> -	.groups = ledtrig_disk_groups,
> -};
> -static struct led_trigger ledtrig_disk_write = {
> -	.name = "disk-write",
> -	.activate = ledtrig_disk_activate,
> -	.groups = ledtrig_disk_groups,
> +static struct ledtrig_disk_trigger ledtrig_disk = {
> +	.all = {
> +		.name = "disk-activity",
> +		.activate = ledtrig_disk_activate,
> +		.groups = ledtrig_disk_groups,
> +	},
> +	.read = {
> +		.name = "disk-read",
> +		.activate = ledtrig_disk_activate,
> +		.groups = ledtrig_disk_groups,
> +	},
> +	.write = {
> +		.name = "disk-write",
> +		.activate = ledtrig_disk_activate,
> +		.groups = ledtrig_disk_groups,
> +	},
>  };
>  
>  static void ledtrig_disk_blink_oneshot(struct led_trigger *trig)
> @@ -189,21 +191,121 @@ static void ledtrig_disk_blink_oneshot(struct led_trigger *trig)
>  	rcu_read_unlock();
>  }
>  
> -void ledtrig_disk_activity(bool write)
> +static void ledtrig_disk_trigger_activity(struct ledtrig_disk_trigger *trig, bool write)
>  {
> -	ledtrig_disk_blink_oneshot(&ledtrig_disk);
> +	if (IS_ERR_OR_NULL(trig))
> +		return;
> +	ledtrig_disk_blink_oneshot(&trig->all);
>  	if (write)
> -		ledtrig_disk_blink_oneshot(&ledtrig_disk_write);
> +		ledtrig_disk_blink_oneshot(&trig->write);
>  	else
> -		ledtrig_disk_blink_oneshot(&ledtrig_disk_read);
> +		ledtrig_disk_blink_oneshot(&trig->read);
> +}
> +
> +void ledtrig_disk_activity(struct ledtrig_disk_trigger *port, bool write)
> +{
> +	ledtrig_disk_trigger_activity(&ledtrig_disk, write);
> +	ledtrig_disk_trigger_activity(port, write);
>  }
>  EXPORT_SYMBOL(ledtrig_disk_activity);
>  
> +struct ledtrig_disk_trigger *ledtrig_disk_trigger_register(const char *name)
> +{
> +	struct ledtrig_disk_trigger *trigger = kzalloc(sizeof(*trigger), GFP_KERNEL);
> +	int ret, n;
> +
> +	if (!trigger)
> +		return ERR_PTR(-ENOMEM);
> +
> +	trigger->all.name = kzalloc(TRIG_NAME_MAX, GFP_KERNEL);
> +	if (!trigger->all.name) {
> +		ret = -ENOMEM;
> +		goto err1;
> +	}
> +
> +	n = snprintf((char *)trigger->all.name, TRIG_NAME_MAX, "%s-disk-activity", name);
> +	if (n >= TRIG_NAME_MAX) {
> +		ret = -E2BIG;
> +		goto err1;
> +	}
> +
> +	trigger->all.activate = ledtrig_disk_activate;
> +	trigger->all.groups = ledtrig_disk_groups;
> +
> +	ret = led_trigger_register(&trigger->all);
> +	if (ret)
> +		goto err1;
> +
> +	trigger->read.name = kzalloc(TRIG_NAME_MAX, GFP_KERNEL);
> +	if (!trigger->read.name) {
> +		ret = -ENOMEM;
> +		goto err2;
> +	}
> +
> +	n = snprintf((char *)trigger->read.name, TRIG_NAME_MAX, "%s-disk-read", name);
> +	if (n >= TRIG_NAME_MAX) {
> +		ret = -E2BIG;
> +		goto err2;
> +	}
> +
> +	trigger->read.activate = ledtrig_disk_activate;
> +	trigger->read.groups = ledtrig_disk_groups;
> +
> +	ret = led_trigger_register(&trigger->read);
> +	if (ret)
> +		goto err2;
> +
> +	trigger->write.name = kzalloc(TRIG_NAME_MAX, GFP_KERNEL);
> +	if (!trigger->write.name) {
> +		ret = -ENOMEM;
> +		goto err3;
> +	}
> +
> +	n = snprintf((char *)trigger->write.name, TRIG_NAME_MAX, "%s-disk-write", name);
> +	if (n >= TRIG_NAME_MAX) {
> +		ret = -E2BIG;
> +		goto err3;
> +	}
> +
> +	trigger->write.activate = ledtrig_disk_activate;
> +	trigger->write.groups = ledtrig_disk_groups;
> +
> +	ret = led_trigger_register(&trigger->write);
> +	if (ret)
> +		goto err3;
> +
> +	return trigger;
> +
> +err3:
> +	led_trigger_unregister(&trigger->read);
> +err2:
> +	led_trigger_unregister(&trigger->all);
> +err1:
> +	kfree(trigger->all.name);
> +	kfree(trigger->read.name);
> +	kfree(trigger->write.name);
> +	kfree(trigger);
> +
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL(ledtrig_disk_trigger_register);
> +
> +void ledtrig_disk_trigger_unregister(struct ledtrig_disk_trigger *trig)
> +{
> +	if (IS_ERR_OR_NULL(trig))
> +		return;
> +
> +	led_trigger_unregister(&trig->all);
> +	led_trigger_unregister(&trig->read);
> +	led_trigger_unregister(&trig->write);
> +}
> +EXPORT_SYMBOL(ledtrig_disk_trigger_unregister);
> +
>  static int __init ledtrig_disk_init(void)
>  {
> -	led_trigger_register(&ledtrig_disk);
> -	led_trigger_register(&ledtrig_disk_read);
> -	led_trigger_register(&ledtrig_disk_write);
> +	led_trigger_register(&ledtrig_disk.all);
> +	led_trigger_register(&ledtrig_disk.read);
> +	led_trigger_register(&ledtrig_disk.write);
>  
>  	return 0;
>  }
> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> index bcecb4911da9..8841850684f7 100644
> --- a/drivers/scsi/libsas/sas_ata.c
> +++ b/drivers/scsi/libsas/sas_ata.c
> @@ -587,14 +587,13 @@ int sas_ata_init(struct domain_device *found_dev)
>  
>  	ata_host_init(ata_host, ha->dev, &sas_sata_ops);
>  
> -	ap = ata_port_alloc(ata_host);
> +	ap = ata_port_alloc(ata_host, 0);
>  	if (!ap) {
>  		pr_err("ata_port_alloc failed.\n");
>  		rc = -ENODEV;
>  		goto free_host;
>  	}
>  
> -	ap->port_no = 0;
>  	ap->pio_mask = ATA_PIO4;
>  	ap->mwdma_mask = ATA_MWDMA2;
>  	ap->udma_mask = ATA_UDMA6;
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index b16b803cc1ac..3221be97e9c0 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -619,10 +619,22 @@ enum led_trigger_netdev_modes {
>  };
>  
>  /* Trigger specific functions */
> +struct ledtrig_disk_trigger {
> +	struct led_trigger all;
> +	struct led_trigger read;
> +	struct led_trigger write;
> +};
>  #ifdef CONFIG_LEDS_TRIGGER_DISK
> -void ledtrig_disk_activity(bool write);
> +struct ledtrig_disk_trigger *ledtrig_disk_trigger_register(const char *name);
> +void ledtrig_disk_trigger_unregister(struct ledtrig_disk_trigger *trig);
> +void ledtrig_disk_activity(struct ledtrig_disk_trigger *port, bool write);
>  #else
> -static inline void ledtrig_disk_activity(bool write) {}
> +static inline struct ledtrig_disk_trigger *ledtrig_disk_trigger_register(const char *name)
> +{
> +	return ERR_PTR(-EOPNOTSUPP);
> +}
> +static inline void ledtrig_disk_trigger_unregister(struct ledtrig_disk_trigger *trig) {}
> +static inline void ledtrig_disk_activity(struct ledtrig_disk_trigger *port, bool write) {}
>  #endif
>  
>  #ifdef CONFIG_LEDS_TRIGGER_MTD
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 39534fafa36a..50124d170d13 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -940,6 +940,10 @@ struct ata_port {
>  #ifdef CONFIG_ATA_ACPI
>  	struct ata_acpi_gtm	__acpi_init_gtm; /* use ata_acpi_init_gtm() */
>  #endif
> +
> +#ifdef CONFIG_LEDS_TRIGGER_DISK
> +	struct ledtrig_disk_trigger *led_trigger;
> +#endif
>  };
>  
>  /* The following initializer overrides a method to NULL whether one of
> @@ -1307,7 +1311,7 @@ extern int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy,
>  			     bool spm_wakeup);
>  extern int ata_slave_link_init(struct ata_port *ap);
>  extern void ata_port_probe(struct ata_port *ap);
> -extern struct ata_port *ata_port_alloc(struct ata_host *host);
> +extern struct ata_port *ata_port_alloc(struct ata_host *host, int port_no);
>  extern void ata_port_free(struct ata_port *ap);
>  extern int ata_tport_add(struct device *parent, struct ata_port *ap);
>  extern void ata_tport_delete(struct ata_port *ap);
> 


-- 
Damien Le Moal
Western Digital Research

Powered by blists - more mailing lists