On Thu, Jan 03, 2008 at 04:55:49PM +0100, Martin Michlmayr wrote:
> Adding Russell King to CC who can hopefully answer James' question
> below.
>
> * James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> [2008-01-03 09:21]:
> > On Thu, 2008-01-03 at 08:43 +0100, Martin Michlmayr wrote:
> > > Commit 9d511a4b29de6764931343d03e493f2e04df0271 removed the "depends
> > > on BROKEN || X86_32" line from advansys' Kconfig entry. I'm not sure
> > > that was such a great idea - the module doesn't compile at least on
> > > ARM and MIPS:
> > >
> > > ARM:
> > >
> > > CC [M] drivers/scsi/advansys.o
> > > drivers/scsi/advansys.c:71:2: warning: #warning this driver is still not properly converted to the DMA
> > > API
> > > drivers/scsi/advansys.c: In function ‘AdvBuildCarrierFreelist’:
> > > drivers/scsi/advansys.c:6486: warning: ‘virt_to_bus’ is deprecated (declared at include/asm/memory.h:1
> > > 92)
> > > drivers/scsi/advansys.c:6486: warning: ‘virt_to_bus’ is deprecated (declared at include/asm/memory.h:1
> > > 92)
> > > drivers/scsi/advansys.c:6486: warning: ‘virt_to_bus’ is deprecated (declared at include/asm/memory.h:1
> > > 92)
> > > ...
> > > drivers/scsi/advansys.c: In function ‘advansys_get_sense_buffer_dma’:
> > > drivers/scsi/advansys.c:9885: error: implicit declaration of function ‘dma_cache_sync’
> >
> > That error doesn't look to tbe the fault of the driver.
> > dma_cache_sync() is a piece of the DMA API it looks like ARM doesn't
> > implement?
Let's look at the code:
scp->SCp.dma_handle = dma_map_single(board->dev, scp->sense_buffer,
sizeof(scp->sense_buffer), DMA_FROM_DEVICE);
dma_cache_sync(board->dev, scp->sense_buffer,
sizeof(scp->sense_buffer), DMA_FROM_DEVICE);
What's the point of that dma_cache_sync() ?
Ah - this is why - sense_buffer could share the same cache line as SCp.
unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE];
/* obtained by REQUEST SENSE when
* CHECK CONDITION is received on original
* command (auto-sense) */
/* Low-level done function - can be used by low-level driver to point
* to completion function. Not used by mid/upper level code. */
void (*scsi_done) (struct scsi_cmnd *);
/*
* The following fields can be written to by the host specific code.
* Everything else should be left alone.
*/
struct scsi_pointer SCp; /* Scratchpad used by some host adapters */
So reading or writing scp->SCp.dma_handle could destroy what
dma_map_single() did.
However, that's not the end of the issue, because if the cache line is
shared, the very next action the driver does:
return cpu_to_le32(scp->SCp.dma_handle);
will result in the cache line being re-loaded into the cache. So even if
we did implement dma_cache_sync(), this driver would still be broken.
The real answer in this case is to fix the driver - even with
dma_cache_sync() implemented, the driver is still broken on ARM. Or fix
the SCSI core problem which causes driver writers to write crap code like
this - like BenH has been trying to do. See the threads on lkml from
BenH:
Subject: [PATCH 1/2] DMA buffer alignment annotations
Subject: [PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd
Fixing that in some way and removing that redundant and improper
dma_cache_sync() will fix the driver properly on ARM. Anything else
is just papering over the problem.
Matthew disagrees with BenH on his method of fixing this - but not that
there is a problem. So this driver can be added to Matthew's list of
"broken drivers that DMA directly to the SCSI command structure". So
Matthew's wrong to globally remove the 'BROKEN' annotation in the Kconfig.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
