Message ID | 20220617125750.728590-3-arnd@kernel.org |
---|---|
State | New |
Headers | show |
Series | phase out CONFIG_VIRT_TO_BUS | expand |
On 2022-06-17 13:57, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > The BusLogic driver is the last remaining driver that relies on the > deprecated bus_to_virt() function, which in turn only works on a few > architectures, and is incompatible with both swiotlb and iommu support. > > Before commit 391e2f25601e ("[SCSI] BusLogic: Port driver to 64-bit."), > the driver had a dependency on x86-32, presumably because of this > problem. However, the change introduced another bug that made it still > impossible to use the driver on any 64-bit machine. > > This was in turn fixed in commit 56f396146af2 ("scsi: BusLogic: Fix > 64-bit system enumeration error for Buslogic"), 8 years later, which > shows that there are not a lot of users. > > Maciej is still using the driver on 32-bit hardware, and Khalid mentioned > that the driver works with the device emulation used in VirtualBox > and VMware. Both of those only emulate it for Windows 2000 and older > operating systems that did not ship with the better LSI logic driver. > > Do a minimum fix that searches through the list of descriptors to find > one that matches the bus address. This is clearly as inefficient as > was indicated in the code comment about the lack of a bus_to_virt() > replacement. A better fix would likely involve changing out the entire > descriptor allocation for a simpler one, but that would be much > more invasive. FWIW, if efficiency *is* a practical concern, even under the current allocation scheme it looks like there are only 4 actual DMA allocations to search through. From a quick scan (since it's too hot here not to get distracted...), if I wanted to optimise this in future I'd probably remove the semi-redundant allocgrp_* fields from struct blogic_ccb and hang a dedicated list of the block allocations off the adapter - at that point the lookup could likely already be more efficient than a theoretical dma_to_virt() interface would be if it had to go off and walk an IOMMU pagetable. Then the next question would be whether it's viable to make a single 32KB allocation rather 4*8KB, so it's no longer even a list. For now, though, I agree with the simple change that's clear and easy to reason about: Reviewed-by: Robin Murphy <robin.murphy@arm.com> Thanks, Robin. > Cc: Maciej W. Rozycki <macro@orcam.me.uk> > Cc: Matt Wang <wwentao@vmware.com> > Cc: Khalid Aziz <khalid@gonehiking.org> > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/scsi/BusLogic.c | 27 ++++++++++++++++----------- > drivers/scsi/Kconfig | 2 +- > 2 files changed, 17 insertions(+), 12 deletions(-) > > diff --git a/drivers/scsi/BusLogic.c b/drivers/scsi/BusLogic.c > index a897c8f914cf..d057abfcdd5c 100644 > --- a/drivers/scsi/BusLogic.c > +++ b/drivers/scsi/BusLogic.c > @@ -2515,12 +2515,26 @@ static int blogic_resultcode(struct blogic_adapter *adapter, > return (hoststatus << 16) | tgt_status; > } > > +/* > + * turn the dma address from an inbox into a ccb pointer > + * This is rather inefficient. > + */ > +static struct blogic_ccb * > +blogic_inbox_to_ccb(struct blogic_adapter *adapter, struct blogic_inbox *inbox) > +{ > + struct blogic_ccb *ccb; > + > + for (ccb = adapter->all_ccbs; ccb; ccb = ccb->next_all) > + if (inbox->ccb == ccb->dma_handle) > + break; > + > + return ccb; > +} > > /* > blogic_scan_inbox scans the Incoming Mailboxes saving any > Incoming Mailbox entries for completion processing. > */ > - > static void blogic_scan_inbox(struct blogic_adapter *adapter) > { > /* > @@ -2540,16 +2554,7 @@ static void blogic_scan_inbox(struct blogic_adapter *adapter) > enum blogic_cmplt_code comp_code; > > while ((comp_code = next_inbox->comp_code) != BLOGIC_INBOX_FREE) { > - /* > - We are only allowed to do this because we limit our > - architectures we run on to machines where bus_to_virt( > - actually works. There *needs* to be a dma_addr_to_virt() > - in the new PCI DMA mapping interface to replace > - bus_to_virt() or else this code is going to become very > - innefficient. > - */ > - struct blogic_ccb *ccb = > - (struct blogic_ccb *) bus_to_virt(next_inbox->ccb); > + struct blogic_ccb *ccb = blogic_inbox_to_ccb(adapter, adapter->next_inbox); > if (comp_code != BLOGIC_CMD_NOTFOUND) { > if (ccb->status == BLOGIC_CCB_ACTIVE || > ccb->status == BLOGIC_CCB_RESET) { > diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig > index cf75588a2587..56bdc08d0b77 100644 > --- a/drivers/scsi/Kconfig > +++ b/drivers/scsi/Kconfig > @@ -513,7 +513,7 @@ config SCSI_HPTIOP > > config SCSI_BUSLOGIC > tristate "BusLogic SCSI support" > - depends on PCI && SCSI && VIRT_TO_BUS > + depends on PCI && SCSI > help > This is support for BusLogic MultiMaster and FlashPoint SCSI Host > Adapters. Consult the SCSI-HOWTO, available from
On 6/17/22 14:57, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > The BusLogic driver is the last remaining driver that relies on the > deprecated bus_to_virt() function, which in turn only works on a few > architectures, and is incompatible with both swiotlb and iommu support. > > Before commit 391e2f25601e ("[SCSI] BusLogic: Port driver to 64-bit."), > the driver had a dependency on x86-32, presumably because of this > problem. However, the change introduced another bug that made it still > impossible to use the driver on any 64-bit machine. > > This was in turn fixed in commit 56f396146af2 ("scsi: BusLogic: Fix > 64-bit system enumeration error for Buslogic"), 8 years later, which > shows that there are not a lot of users. > > Maciej is still using the driver on 32-bit hardware, and Khalid mentioned > that the driver works with the device emulation used in VirtualBox > and VMware. Both of those only emulate it for Windows 2000 and older > operating systems that did not ship with the better LSI logic driver. > > Do a minimum fix that searches through the list of descriptors to find > one that matches the bus address. This is clearly as inefficient as > was indicated in the code comment about the lack of a bus_to_virt() > replacement. A better fix would likely involve changing out the entire > descriptor allocation for a simpler one, but that would be much > more invasive. > > Cc: Maciej W. Rozycki <macro@orcam.me.uk> > Cc: Matt Wang <wwentao@vmware.com> > Cc: Khalid Aziz <khalid@gonehiking.org> > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/scsi/BusLogic.c | 27 ++++++++++++++++----------- > drivers/scsi/Kconfig | 2 +- > 2 files changed, 17 insertions(+), 12 deletions(-) > > diff --git a/drivers/scsi/BusLogic.c b/drivers/scsi/BusLogic.c > index a897c8f914cf..d057abfcdd5c 100644 > --- a/drivers/scsi/BusLogic.c > +++ b/drivers/scsi/BusLogic.c > @@ -2515,12 +2515,26 @@ static int blogic_resultcode(struct blogic_adapter *adapter, > return (hoststatus << 16) | tgt_status; > } > > +/* > + * turn the dma address from an inbox into a ccb pointer > + * This is rather inefficient. > + */ > +static struct blogic_ccb * > +blogic_inbox_to_ccb(struct blogic_adapter *adapter, struct blogic_inbox *inbox) > +{ > + struct blogic_ccb *ccb; > + > + for (ccb = adapter->all_ccbs; ccb; ccb = ccb->next_all) > + if (inbox->ccb == ccb->dma_handle) > + break; > + > + return ccb; > +} > > /* > blogic_scan_inbox scans the Incoming Mailboxes saving any > Incoming Mailbox entries for completion processing. > */ > - > static void blogic_scan_inbox(struct blogic_adapter *adapter) > { > /* > @@ -2540,16 +2554,7 @@ static void blogic_scan_inbox(struct blogic_adapter *adapter) > enum blogic_cmplt_code comp_code; > > while ((comp_code = next_inbox->comp_code) != BLOGIC_INBOX_FREE) { > - /* > - We are only allowed to do this because we limit our > - architectures we run on to machines where bus_to_virt( > - actually works. There *needs* to be a dma_addr_to_virt() > - in the new PCI DMA mapping interface to replace > - bus_to_virt() or else this code is going to become very > - innefficient. > - */ > - struct blogic_ccb *ccb = > - (struct blogic_ccb *) bus_to_virt(next_inbox->ccb); > + struct blogic_ccb *ccb = blogic_inbox_to_ccb(adapter, adapter->next_inbox); > if (comp_code != BLOGIC_CMD_NOTFOUND) { > if (ccb->status == BLOGIC_CCB_ACTIVE || > ccb->status == BLOGIC_CCB_RESET) { > diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig > index cf75588a2587..56bdc08d0b77 100644 > --- a/drivers/scsi/Kconfig > +++ b/drivers/scsi/Kconfig > @@ -513,7 +513,7 @@ config SCSI_HPTIOP > > config SCSI_BUSLOGIC > tristate "BusLogic SCSI support" > - depends on PCI && SCSI && VIRT_TO_BUS > + depends on PCI && SCSI > help > This is support for BusLogic MultiMaster and FlashPoint SCSI Host > Adapters. Consult the SCSI-HOWTO, available from CCB handling in the driver is ugly anyway, so that'll be good enough. Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On 6/17/22 06:57, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > The BusLogic driver is the last remaining driver that relies on the > deprecated bus_to_virt() function, which in turn only works on a few > architectures, and is incompatible with both swiotlb and iommu support. > > Before commit 391e2f25601e ("[SCSI] BusLogic: Port driver to 64-bit."), > the driver had a dependency on x86-32, presumably because of this > problem. However, the change introduced another bug that made it still > impossible to use the driver on any 64-bit machine. > > This was in turn fixed in commit 56f396146af2 ("scsi: BusLogic: Fix > 64-bit system enumeration error for Buslogic"), 8 years later, which > shows that there are not a lot of users. > > Maciej is still using the driver on 32-bit hardware, and Khalid mentioned > that the driver works with the device emulation used in VirtualBox > and VMware. Both of those only emulate it for Windows 2000 and older > operating systems that did not ship with the better LSI logic driver. > > Do a minimum fix that searches through the list of descriptors to find > one that matches the bus address. This is clearly as inefficient as > was indicated in the code comment about the lack of a bus_to_virt() > replacement. A better fix would likely involve changing out the entire > descriptor allocation for a simpler one, but that would be much > more invasive. > > Cc: Maciej W. Rozycki <macro@orcam.me.uk> > Cc: Matt Wang <wwentao@vmware.com> > Cc: Khalid Aziz <khalid@gonehiking.org> > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/scsi/BusLogic.c | 27 ++++++++++++++++----------- > drivers/scsi/Kconfig | 2 +- > 2 files changed, 17 insertions(+), 12 deletions(-) > > diff --git a/drivers/scsi/BusLogic.c b/drivers/scsi/BusLogic.c > index a897c8f914cf..d057abfcdd5c 100644 > --- a/drivers/scsi/BusLogic.c > +++ b/drivers/scsi/BusLogic.c > @@ -2515,12 +2515,26 @@ static int blogic_resultcode(struct blogic_adapter *adapter, > return (hoststatus << 16) | tgt_status; > } > > +/* > + * turn the dma address from an inbox into a ccb pointer > + * This is rather inefficient. > + */ > +static struct blogic_ccb * > +blogic_inbox_to_ccb(struct blogic_adapter *adapter, struct blogic_inbox *inbox) > +{ > + struct blogic_ccb *ccb; > + > + for (ccb = adapter->all_ccbs; ccb; ccb = ccb->next_all) > + if (inbox->ccb == ccb->dma_handle) > + break; > + > + return ccb; > +} > > /* > blogic_scan_inbox scans the Incoming Mailboxes saving any > Incoming Mailbox entries for completion processing. > */ > - > static void blogic_scan_inbox(struct blogic_adapter *adapter) > { > /* > @@ -2540,16 +2554,7 @@ static void blogic_scan_inbox(struct blogic_adapter *adapter) > enum blogic_cmplt_code comp_code; > > while ((comp_code = next_inbox->comp_code) != BLOGIC_INBOX_FREE) { > - /* > - We are only allowed to do this because we limit our > - architectures we run on to machines where bus_to_virt( > - actually works. There *needs* to be a dma_addr_to_virt() > - in the new PCI DMA mapping interface to replace > - bus_to_virt() or else this code is going to become very > - innefficient. > - */ > - struct blogic_ccb *ccb = > - (struct blogic_ccb *) bus_to_virt(next_inbox->ccb); > + struct blogic_ccb *ccb = blogic_inbox_to_ccb(adapter, adapter->next_inbox); This change looks good enough as workaround to not use bus_to_virt() for now. There are two problems I see though. One, I do worry about blogic_inbox_to_ccb() returning NULL for ccb which should not happen unless the mailbox pointer was corrupted which would indicate a bigger problem. Nevertheless a NULL pointer causing kernel panic concerns me. How about adding a check before we dereference ccb? Second, with this patch applied, I am seeing errors from the driver: ===================== [ 1623.902685] sdb: sdb1 sdb2 [ 1623.903245] sd 2:0:0:0: [sdb] Attached SCSI disk [ 1623.911000] scsi2: Illegal CCB #76 status 2 in Incoming Mailbox [ 1623.911005] scsi2: Illegal CCB #76 status 2 in Incoming Mailbox [ 1623.911070] scsi2: Illegal CCB #79 status 2 in Incoming Mailbox [ 1651.458008] scsi2: Warning: Partition Table appears to have Geometry 256/63 which is [ 1651.458013] scsi2: not compatible with current BusLogic Host Adapter Geometry 255/63 [ 1658.797609] scsi2: Resetting BusLogic BT-958D Failed [ 1659.533208] sd 2:0:0:0: Device offlined - not ready after error recovery [ 1659.533331] sd 2:0:0:0: Device offlined - not ready after error recovery [ 1659.533333] sd 2:0:0:0: Device offlined - not ready after error recovery [ 1659.533342] sd 2:0:0:0: [sdb] tag#101 FAILED Result: hostbyte=DID_TIME_OUT driverbyte=DRIVER_OK cmd_age=35s [ 1659.533345] sd 2:0:0:0: [sdb] tag#101 CDB: Read(10) 28 00 00 00 00 28 00 00 10 00 [ 1659.533346] I/O error, dev sdb, sector 40 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 0 ================= This is on VirtualBox using emulated BusLogic adapter. This patch needs more refinement. Thanks, Khalid > if (comp_code != BLOGIC_Cn erroneousMD_NOTFOUND) { > if (ccb->status == BLOGIC_CCB_ACTIVE || > ccb->status == BLOGIC_CCB_RESET) { > diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig > index cf75588a2587..56bdc08d0b77 100644 > --- a/drivers/scsi/Kconfig > +++ b/drivers/scsi/Kconfig > @@ -513,7 +513,7 @@ config SCSI_HPTIOP > > config SCSI_BUSLOGIC > tristate "BusLogic SCSI support" > - depends on PCI && SCSI && VIRT_TO_BUS > + depends on PCI && SCSI > help > This is support for BusLogic MultiMaster and FlashPoint SCSI Host > Adapters. Consult the SCSI-HOWTO, available from
On Tue, Jun 21, 2022 at 11:56 PM Khalid Aziz <khalid@gonehiking.org> wrote: > > while ((comp_code = next_inbox->comp_code) != BLOGIC_INBOX_FREE) { > > - /* > > - We are only allowed to do this because we limit our > > - architectures we run on to machines where bus_to_virt( > > - actually works. There *needs* to be a dma_addr_to_virt() > > - in the new PCI DMA mapping interface to replace > > - bus_to_virt() or else this code is going to become very > > - innefficient. > > - */ > > - struct blogic_ccb *ccb = > > - (struct blogic_ccb *) bus_to_virt(next_inbox->ccb); > > + struct blogic_ccb *ccb = blogic_inbox_to_ccb(adapter, adapter->next_inbox); > > This change looks good enough as workaround to not use bus_to_virt() for > now. There are two problems I see though. One, I do worry about > blogic_inbox_to_ccb() returning NULL for ccb which should not happen > unless the mailbox pointer was corrupted which would indicate a bigger > problem. Nevertheless a NULL pointer causing kernel panic concerns me. > How about adding a check before we dereference ccb? Right, makes sense > Second, with this patch applied, I am seeing errors from the driver: > > ===================== > [ 1623.902685] sdb: sdb1 sdb2 > [ 1623.903245] sd 2:0:0:0: [sdb] Attached SCSI disk > [ 1623.911000] scsi2: Illegal CCB #76 status 2 in Incoming Mailbox > [ 1623.911005] scsi2: Illegal CCB #76 status 2 in Incoming Mailbox > [ 1623.911070] scsi2: Illegal CCB #79 status 2 in Incoming Mailbox > [ 1651.458008] scsi2: Warning: Partition Table appears to have Geometry > 256/63 which is > [ 1651.458013] scsi2: not compatible with current BusLogic Host Adapter > Geometry 255/63 > [ 1658.797609] scsi2: Resetting BusLogic BT-958D Failed > [ 1659.533208] sd 2:0:0:0: Device offlined - not ready after error recovery > [ 1659.533331] sd 2:0:0:0: Device offlined - not ready after error recovery > [ 1659.533333] sd 2:0:0:0: Device offlined - not ready after error recovery > [ 1659.533342] sd 2:0:0:0: [sdb] tag#101 FAILED Result: > hostbyte=DID_TIME_OUT driverbyte=DRIVER_OK cmd_age=35s > [ 1659.533345] sd 2:0:0:0: [sdb] tag#101 CDB: Read(10) 28 00 00 00 00 28 > 00 00 10 00 > [ 1659.533346] I/O error, dev sdb, sector 40 op 0x0:(READ) flags 0x80700 > phys_seg 1 prio class 0 > > ================= > > This is on VirtualBox using emulated BusLogic adapter. > > This patch needs more refinement. Thanks for testing the patch, too bad it didn't work. At least I quickly found one stupid mistake on my end, hope it's the only one. Can you test it again with this patch on top? diff --git a/drivers/scsi/BusLogic.c b/drivers/scsi/BusLogic.c index d057abfcdd5c..9e67f2ee25ee 100644 --- a/drivers/scsi/BusLogic.c +++ b/drivers/scsi/BusLogic.c @@ -2554,8 +2554,14 @@ static void blogic_scan_inbox(struct blogic_adapter *adapter) enum blogic_cmplt_code comp_code; while ((comp_code = next_inbox->comp_code) != BLOGIC_INBOX_FREE) { - struct blogic_ccb *ccb = blogic_inbox_to_ccb(adapter, adapter->next_inbox); - if (comp_code != BLOGIC_CMD_NOTFOUND) { + struct blogic_ccb *ccb = blogic_inbox_to_ccb(adapter, next_inbox); + if (!ccb) { + /* + * This should never happen, unless the CCB list is + * corrupted in memory. + */ + blogic_warn("Could not find CCB for dma address 0x%x\n", adapter, next_inbox->ccb); + } else if (comp_code != BLOGIC_CMD_NOTFOUND) { if (ccb->status == BLOGIC_CCB_ACTIVE || ccb->status == BLOGIC_CCB_RESET) {
On 6/23/22 08:47, Arnd Bergmann wrote: > > > Can you test it again with this patch on top? > > diff --git a/drivers/scsi/BusLogic.c b/drivers/scsi/BusLogic.c > index d057abfcdd5c..9e67f2ee25ee 100644 > --- a/drivers/scsi/BusLogic.c > +++ b/drivers/scsi/BusLogic.c > @@ -2554,8 +2554,14 @@ static void blogic_scan_inbox(struct > blogic_adapter *adapter) > enum blogic_cmplt_code comp_code; > > while ((comp_code = next_inbox->comp_code) != BLOGIC_INBOX_FREE) { > - struct blogic_ccb *ccb = blogic_inbox_to_ccb(adapter, > adapter->next_inbox); > - if (comp_code != BLOGIC_CMD_NOTFOUND) { > + struct blogic_ccb *ccb = blogic_inbox_to_ccb(adapter, > next_inbox); > + if (!ccb) { > + /* > + * This should never happen, unless the CCB list is > + * corrupted in memory. > + */ > + blogic_warn("Could not find CCB for dma > address 0x%x\n", adapter, next_inbox->ccb); > + } else if (comp_code != BLOGIC_CMD_NOTFOUND) { > if (ccb->status == BLOGIC_CCB_ACTIVE || > ccb->status == BLOGIC_CCB_RESET) { Hi Arnd, Driver works with this change. next_inbox is the correct pointer to pass. Thanks, Khalid
On Fri, Jun 24, 2022 at 5:38 PM Khalid Aziz <khalid@gonehiking.org> wrote: > On 6/23/22 08:47, Arnd Bergmann wrote: > > Driver works with this change. next_inbox is the correct pointer to pass. Ok, great! I'll add a 'Tested-by' line then. I was already in the process of preparing a v3 patch set, will send out the fixed patch in a bit. Arnd
diff --git a/drivers/scsi/BusLogic.c b/drivers/scsi/BusLogic.c index a897c8f914cf..d057abfcdd5c 100644 --- a/drivers/scsi/BusLogic.c +++ b/drivers/scsi/BusLogic.c @@ -2515,12 +2515,26 @@ static int blogic_resultcode(struct blogic_adapter *adapter, return (hoststatus << 16) | tgt_status; } +/* + * turn the dma address from an inbox into a ccb pointer + * This is rather inefficient. + */ +static struct blogic_ccb * +blogic_inbox_to_ccb(struct blogic_adapter *adapter, struct blogic_inbox *inbox) +{ + struct blogic_ccb *ccb; + + for (ccb = adapter->all_ccbs; ccb; ccb = ccb->next_all) + if (inbox->ccb == ccb->dma_handle) + break; + + return ccb; +} /* blogic_scan_inbox scans the Incoming Mailboxes saving any Incoming Mailbox entries for completion processing. */ - static void blogic_scan_inbox(struct blogic_adapter *adapter) { /* @@ -2540,16 +2554,7 @@ static void blogic_scan_inbox(struct blogic_adapter *adapter) enum blogic_cmplt_code comp_code; while ((comp_code = next_inbox->comp_code) != BLOGIC_INBOX_FREE) { - /* - We are only allowed to do this because we limit our - architectures we run on to machines where bus_to_virt( - actually works. There *needs* to be a dma_addr_to_virt() - in the new PCI DMA mapping interface to replace - bus_to_virt() or else this code is going to become very - innefficient. - */ - struct blogic_ccb *ccb = - (struct blogic_ccb *) bus_to_virt(next_inbox->ccb); + struct blogic_ccb *ccb = blogic_inbox_to_ccb(adapter, adapter->next_inbox); if (comp_code != BLOGIC_CMD_NOTFOUND) { if (ccb->status == BLOGIC_CCB_ACTIVE || ccb->status == BLOGIC_CCB_RESET) { diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig index cf75588a2587..56bdc08d0b77 100644 --- a/drivers/scsi/Kconfig +++ b/drivers/scsi/Kconfig @@ -513,7 +513,7 @@ config SCSI_HPTIOP config SCSI_BUSLOGIC tristate "BusLogic SCSI support" - depends on PCI && SCSI && VIRT_TO_BUS + depends on PCI && SCSI help This is support for BusLogic MultiMaster and FlashPoint SCSI Host Adapters. Consult the SCSI-HOWTO, available from