| Message ID | 20240626180031.4050226-26-cassel@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ata,libsas: Assign the unique id used for printing earlier | expand |
On 6/27/24 03:00, Niklas Cassel wrote: > Currently, the ata_port print_ids are increased indefinitely, even when > there are lower ids available. > > E.g. on first boot you will have ata1-ata6 assigned. > After a rmmod + modprobe, you will instead have ata7-ata12 assigned. > > Move to use the ida_alloc() API, such that print_ids will get reused. > This means that even after a rmmod + modprobe, the ports will be assigned > print_ids ata1-ata6. > > Signed-off-by: Niklas Cassel <cassel@kernel.org> Looks good. But maybe it would make sense to squash this together with patch 10 ? Either way, Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
On 6/26/24 20:00, Niklas Cassel wrote: > Currently, the ata_port print_ids are increased indefinitely, even when > there are lower ids available. > > E.g. on first boot you will have ata1-ata6 assigned. > After a rmmod + modprobe, you will instead have ata7-ata12 assigned. > > Move to use the ida_alloc() API, such that print_ids will get reused. > This means that even after a rmmod + modprobe, the ports will be assigned > print_ids ata1-ata6. > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > --- > drivers/ata/libata-core.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
Hi Niklas,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.10-rc5 next-20240627]
[cannot apply to mkp-scsi/for-next jejb-scsi/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Niklas-Cassel/ata-libata-core-Fix-null-pointer-dereference-on-error/20240627-123023
base: linus/master
patch link: https://lore.kernel.org/r/20240626180031.4050226-26-cassel%40kernel.org
patch subject: [PATCH v2 11/13] ata: libata-core: Reuse available ata_port print_ids
config: i386-randconfig-141-20240628 (https://download.01.org/0day-ci/archive/20240629/202406290027.cdgsPQAF-lkp@intel.com/config)
compiler: gcc-8 (Ubuntu 8.4.0-3ubuntu2) 8.4.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406290027.cdgsPQAF-lkp@intel.com/
smatch warnings:
drivers/ata/libata-core.c:5467 ata_port_alloc() warn: unsigned 'ap->print_id' is never less than zero.
vim +5467 drivers/ata/libata-core.c
5443
5444 /**
5445 * ata_port_alloc - allocate and initialize basic ATA port resources
5446 * @host: ATA host this allocated port belongs to
5447 *
5448 * Allocate and initialize basic ATA port resources.
5449 *
5450 * RETURNS:
5451 * Allocate ATA port on success, NULL on failure.
5452 *
5453 * LOCKING:
5454 * Inherited from calling layer (may sleep).
5455 */
5456 struct ata_port *ata_port_alloc(struct ata_host *host)
5457 {
5458 struct ata_port *ap;
5459
5460 ap = kzalloc(sizeof(*ap), GFP_KERNEL);
5461 if (!ap)
5462 return NULL;
5463
5464 ap->pflags |= ATA_PFLAG_INITIALIZING | ATA_PFLAG_FROZEN;
5465 ap->lock = &host->lock;
5466 ap->print_id = ida_alloc_min(&ata_ida, 1, GFP_KERNEL);
> 5467 if (ap->print_id < 0) {
5468 kfree(ap);
5469 return NULL;
5470 }
5471 ap->host = host;
5472 ap->dev = host->dev;
5473
5474 mutex_init(&ap->scsi_scan_mutex);
5475 INIT_DELAYED_WORK(&ap->hotplug_task, ata_scsi_hotplug);
5476 INIT_DELAYED_WORK(&ap->scsi_rescan_task, ata_scsi_dev_rescan);
5477 INIT_LIST_HEAD(&ap->eh_done_q);
5478 init_waitqueue_head(&ap->eh_wait_q);
5479 init_completion(&ap->park_req_pending);
5480 timer_setup(&ap->fastdrain_timer, ata_eh_fastdrain_timerfn,
5481 TIMER_DEFERRABLE);
5482
5483 ap->cbl = ATA_CBL_NONE;
5484
5485 ata_link_init(ap, &ap->link, 0);
5486
On Sat, Jun 29, 2024 at 12:31:57AM +0800, kernel test robot wrote: > Hi Niklas, > > kernel test robot noticed the following build warnings: > > [auto build test WARNING on linus/master] > [also build test WARNING on v6.10-rc5 next-20240627] > [cannot apply to mkp-scsi/for-next jejb-scsi/for-next] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Niklas-Cassel/ata-libata-core-Fix-null-pointer-dereference-on-error/20240627-123023 > base: linus/master > patch link: https://lore.kernel.org/r/20240626180031.4050226-26-cassel%40kernel.org > patch subject: [PATCH v2 11/13] ata: libata-core: Reuse available ata_port print_ids > config: i386-randconfig-141-20240628 (https://download.01.org/0day-ci/archive/20240629/202406290027.cdgsPQAF-lkp@intel.com/config) > compiler: gcc-8 (Ubuntu 8.4.0-3ubuntu2) 8.4.0 > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202406290027.cdgsPQAF-lkp@intel.com/ > > smatch warnings: > drivers/ata/libata-core.c:5467 ata_port_alloc() warn: unsigned 'ap->print_id' is never less than zero. > > vim +5467 drivers/ata/libata-core.c > > 5443 > 5444 /** > 5445 * ata_port_alloc - allocate and initialize basic ATA port resources > 5446 * @host: ATA host this allocated port belongs to > 5447 * > 5448 * Allocate and initialize basic ATA port resources. > 5449 * > 5450 * RETURNS: > 5451 * Allocate ATA port on success, NULL on failure. > 5452 * > 5453 * LOCKING: > 5454 * Inherited from calling layer (may sleep). > 5455 */ > 5456 struct ata_port *ata_port_alloc(struct ata_host *host) > 5457 { > 5458 struct ata_port *ap; > 5459 > 5460 ap = kzalloc(sizeof(*ap), GFP_KERNEL); > 5461 if (!ap) > 5462 return NULL; > 5463 > 5464 ap->pflags |= ATA_PFLAG_INITIALIZING | ATA_PFLAG_FROZEN; > 5465 ap->lock = &host->lock; > 5466 ap->print_id = ida_alloc_min(&ata_ida, 1, GFP_KERNEL); > > 5467 if (ap->print_id < 0) { Well, the check is correct, but since ap->print_id is unsigned int, we will need to use a temporary (signed) variable to check for errors from ida_alloc_min(). Will fix in next revision. > 5468 kfree(ap); > 5469 return NULL; > 5470 } > 5471 ap->host = host; > 5472 ap->dev = host->dev; > 5473 > 5474 mutex_init(&ap->scsi_scan_mutex); > 5475 INIT_DELAYED_WORK(&ap->hotplug_task, ata_scsi_hotplug); > 5476 INIT_DELAYED_WORK(&ap->scsi_rescan_task, ata_scsi_dev_rescan); > 5477 INIT_LIST_HEAD(&ap->eh_done_q); > 5478 init_waitqueue_head(&ap->eh_wait_q); > 5479 init_completion(&ap->park_req_pending); > 5480 timer_setup(&ap->fastdrain_timer, ata_eh_fastdrain_timerfn, > 5481 TIMER_DEFERRABLE); > 5482 > 5483 ap->cbl = ATA_CBL_NONE; > 5484 > 5485 ata_link_init(ap, &ap->link, 0); > 5486 > > -- > 0-DAY CI Kernel Test Service > https://github.com/intel/lkp-tests/wiki
On Thu, Jun 27, 2024 at 10:37:40AM +0900, Damien Le Moal wrote: > On 6/27/24 03:00, Niklas Cassel wrote: > > Currently, the ata_port print_ids are increased indefinitely, even when > > there are lower ids available. > > > > E.g. on first boot you will have ata1-ata6 assigned. > > After a rmmod + modprobe, you will instead have ata7-ata12 assigned. > > > > Move to use the ida_alloc() API, such that print_ids will get reused. > > This means that even after a rmmod + modprobe, the ports will be assigned > > print_ids ata1-ata6. > > > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > Looks good. But maybe it would make sense to squash this together with patch 10 ? Patch 10 initializes the print_ids earlier (which is a perfectly fine change on its own, even with the old way to assign print_ids), while this patch changes for the print_ids to be reusable. I think that logically, it is two different logical changes so I will keep them as separate patches in v3. Kind regards, Niklas
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 52c1f0915aef..846ab99e0cd3 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -86,7 +86,7 @@ static unsigned int ata_dev_set_xfermode(struct ata_device *dev); static void ata_dev_xfermask(struct ata_device *dev); static unsigned long ata_dev_blacklisted(const struct ata_device *dev); -atomic_t ata_print_id = ATOMIC_INIT(0); +static DEFINE_IDA(ata_ida); #ifdef CONFIG_ATA_FORCE struct ata_force_param { @@ -5463,7 +5463,11 @@ struct ata_port *ata_port_alloc(struct ata_host *host) ap->pflags |= ATA_PFLAG_INITIALIZING | ATA_PFLAG_FROZEN; ap->lock = &host->lock; - ap->print_id = atomic_inc_return(&ata_print_id); + ap->print_id = ida_alloc_min(&ata_ida, 1, GFP_KERNEL); + if (ap->print_id < 0) { + kfree(ap); + return NULL; + } ap->host = host; ap->dev = host->dev; @@ -5497,6 +5501,7 @@ void ata_port_free(struct ata_port *ap) kfree(ap->pmp_link); kfree(ap->slave_link); kfree(ap->ncq_sense_buf); + ida_free(&ata_ida, ap->print_id); kfree(ap); } EXPORT_SYMBOL_GPL(ata_port_free);
Currently, the ata_port print_ids are increased indefinitely, even when there are lower ids available. E.g. on first boot you will have ata1-ata6 assigned. After a rmmod + modprobe, you will instead have ata7-ata12 assigned. Move to use the ida_alloc() API, such that print_ids will get reused. This means that even after a rmmod + modprobe, the ports will be assigned print_ids ata1-ata6. Signed-off-by: Niklas Cassel <cassel@kernel.org> --- drivers/ata/libata-core.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)