On 17.05.21 16:11, Jan Beulich wrote:
On 13.05.2021 12:02, Juergen Gross wrote:@@ -1574,10 +1580,16 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id) spin_lock_irqsave(&rinfo->ring_lock, flags); again: rp = rinfo->ring.sring->rsp_prod; + if (RING_RESPONSE_PROD_OVERFLOW(&rinfo->ring, rp)) { + pr_alert("%s: illegal number of responses %u\n", + info->gd->disk_name, rp - rinfo->ring.rsp_cons); + goto err; + } rmb(); /* Ensure we see queued responses up to 'rp'. */I think you want to insert after the barrier.
Why? The relevant variable which is checked is "rp". The result of the check is in no way depending on the responses themselves. And any change of rsp_cons is protected by ring_lock, so there is no possibility of reading an old value here.
@@ -1680,6 +1707,11 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id) spin_unlock_irqrestore(&rinfo->ring_lock, flags);return IRQ_HANDLED;+ + err: + info->connected = BLKIF_STATE_ERROR; + pr_alert("%s disabled for further use\n", info->gd->disk_name); + return IRQ_HANDLED; }Am I understanding that a suspend (and then resume) can be used to recover from error state? If so - is this intentional? If so in turn, would it make sense to spell this out in the description?
I'd call it a nice side effect rather than intention. I can add a remark to the commit message if you want. Juergen
Attachment:
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
