On 5/28/20 7:48 PM, Bart Van Assche wrote:
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?
Ok, will do for the next round.
Cheers,
Hannes
