On 2020-05-28 09:36, Hannes Reinecke wrote:
> +#define scsi_target_index(s) \
> + ((((unsigned long)(s)->channel) << 16) | (s)->id)
Please define scsi_target_index() as an inline function instead of a macro.
> + if (xa_insert(&shost->__targets, tid, starget, GFP_KERNEL)) {
> + dev_printk(KERN_ERR, dev, "target index busy\n");
> + kfree(starget);
> + return NULL;
> + }
So the above code passes GFP_KERNEL to xa_insert() while holding a
spinlock? That doesn't seem correct to me. Since xa_insert() provides
locking itself, can the spin_lock_irqsave(shost->host_lock, flags) /
spin_unlock_irqrestore(shost->host_lock, flags) pair be removed and can
the xa_load() and xa_insert() calls be combined into a single
xa_insert() call? I think xa_insert() returns -EBUSY if an entry already
exists.
> -restart:
> spin_lock_irqsave(shost->host_lock, flags);
> - list_for_each_entry(starget, &shost->__targets, siblings) {
> + starget = xa_find(&shost->__targets, &tid, ULONG_MAX, XA_PRESENT);
> + while (starget) {
> if (starget->state == STARGET_DEL ||
> starget->state == STARGET_REMOVE ||
> - starget->state == STARGET_CREATED_REMOVE)
> + starget->state == STARGET_CREATED_REMOVE) {
> + starget = xa_find_after(&shost->__targets, &tid,
> + ULONG_MAX, XA_PRESENT);
> continue;
> + }
> if (starget->dev.parent == dev || &starget->dev == dev) {
> kref_get(&starget->reap_ref);
> if (starget->state == STARGET_CREATED)
> @@ -1530,7 +1534,10 @@ void scsi_remove_target(struct device *dev)
> spin_unlock_irqrestore(shost->host_lock, flags);
> __scsi_remove_target(starget);
> scsi_target_reap(starget);
> - goto restart;
> + spin_lock_irqsave(shost->host_lock, flags);
> + starget = xa_find_after(&shost->__targets, &tid,
> + ULONG_MAX, XA_PRESENT);
> + continue;
> }
> }
How about using a for loop instead of a while loop such that the
xa_find_after() statement does not have to be duplicated?
Thanks,
Bart.
