Is the "megaraid" driver still actively used and maintained? I originally
posted this series on 06.07.2013 and after receiving no comments, pinged
the list again on 06.17.2013 and still received no comments/feedback.
Trying again as I believe there is a real issue here, which I'd like
confirmation on, and we really should remove the local copy/usage of
'struct pci_dev' that this driver currently maintains.
While the megaraid device itself may be 64-bit DMA capable, 32-bit address
restricted DMA buffers are apparently required for "internal commands" as
is denoted by a couple of comments - "For all internal commands, the
buffer must be allocated in <4GB address range" - within the driver.
If the device is 64-bit DMA capable then, once it is setup, any subsequent
DMA allocations for "internal commands" would not be properly restricted
due to megaraid_probe_one() having called pci_set_dma_mask() on pdev with
DMA_BIT_MASK(64). The driver attempts to solve this by using
make_local_pdev() to dynamically create local pci_dev structures which are
then set and used for allocating 32-bit address space restricted DMA
buffers[1] but I don't believe that the implementation works as intended.
Assume that the megaraid device is 64-bit DMA capable. While probing the
device and attaching the megaraid driver, pci_set_dma_mask() is called
with the "originating pdev" and a DMA_BIT_MASK of 64. As a result, any
subsequent dynamic DMA related allocations associated with the
"originating pdev" will acquire 64-bit based buffers, which do not meet
the addressing restrictions for internal commands.
megaraid_probe_one(struct pci_dev *pdev, ...)
...
pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
As mentioned, the driver attempts to solve this by using make_local_pdev()
to dynamically create local pci_dev structures - "local pdev's" - which
are set with a DMA_BIT_MASK of 32.
make_local_pdev
alloc_pci_dev
memcpy
pci_set_dma_mask
dma_set_mask
*dev->dma_mask = mask;
The "local pdev" is then used in allocating a DMA buffer in an attempt to
meet the < 4 GB restriction.
For a 64-bit DMA capable device, the "originating pdev" will have its
'dma_mask' set to 0xffffffffffffffff after the driver attaches.
Subsequently, when an internal command is initiated, make_local_pdev() is
called. make_local_pdev() uses the PCI's core to allocate a "local pdev"
and then copies the "originating pdev" content into the newly allocated
"local pdev". As a result of copying the "originating pdev" content into
the "local pdev", pdev->dev.dma_mask will be pointing back to the
"originating pdev's" 'dma_mask' member, not the "local pdev's" as
intended. Thus, when make_local_pdev() calls pci_set_dma_mask() in an
attempt to set the "local pdev's" DMA mask to 32 it will instead overwrite
the "originating pdev's" DMA mask. Thus, after any user initiated
commands are issued, all subsequent DMA allocations will be 32-bit
restricted from that point onward regardless of whether they are internal
commands or otherwise.
This patch fixes the issue by removing the setup of DMA_BIT_MASK to 64 in
megaraid_probe_one(), leaving the driver with default 32-bit DMA
capabilities, as it currently ends up in such a state anyway after any
internal commands are initiated.
[1] It seems strange that both mega_buffer/buf_dma_handle and
make_local_pdev() both exist for internal commands but this has been
the case for a long time - at least since 2.6.12-rc2. Perhaps there
is some coalescing that could be done.
---
Myron Stowe (3):
[SCSI] megaraid: Remove 64-bit DMA related dead code
[SCSI] megaraid: Remove local pdev's
[SCSI] megaraid: Remove 64-bit DMA_BIT_MASK capability
drivers/scsi/megaraid.c | 152 ++++++++---------------------------------------
drivers/scsi/megaraid.h | 1
2 files changed, 27 insertions(+), 126 deletions(-)
--
If the megaraid device is 64-bit DMA capable then, once it is setup, any
subsequent DMA allocations for "internal commands" would not be properly
restricted due to megaraid_probe_one() having called pci_set_dma_mask() on
pdev with DMA_BIT_MASK(64). The driver attempts to solve this by using
make_local_pdev() to dynamically create local pci_dev structures which are
then set and used for allocating 32-bit address space restricted DMA
buffers but I don't believe that the implementation works as intended.
For a 64-bit DMA capable device, the "originating pdev" will have its
'dma_mask' set to 0xffffffffffffffff after the driver attaches.
Subsequently, when an internal command is initiated, make_local_pdev() is
called. make_local_pdev() uses the PCI's core to allocate a "local pdev"
and then copies the "originating pdev" content into the newly allocated
"local pdev". As a result of copying the "originating pdev" content into
the "local pdev", pdev->dev.dma_mask will be pointing back to the
"originating pdev's" 'dma_mask' member, not the "local pdev's" as
intended. Thus, when make_local_pdev() calls pci_set_dma_mask() in an
attempt to set the "local pdev's" DMA mask to 32 it will instead overwrite
the "originating pdev's" DMA mask. So, after any user initiated commands
are issued, all subsequent DMA allocations will be 32-bit restricted from
that point onward regardless of whether they are internal commands or
otherwise.
This patch fixes the issue by removing the setup of DMA_BIT_MASK to 64 in
megaraid_probe_one(), leaving the driver setup for default 32-bit DMA
capabilities, as it currently ends up in such a state anyway after any
internal commands are initiated.
Signed-off-by: Myron Stowe <[email protected]>
---
drivers/scsi/megaraid.c | 11 +++--------
1 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
index 846f475..32cca61 100644
--- a/drivers/scsi/megaraid.c
+++ b/drivers/scsi/megaraid.c
@@ -4535,14 +4535,9 @@ megaraid_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
mcontroller[i].uid = (pci_bus << 8) | pci_dev_func;
- /* Set the Mode of addressing to 64 bit if we can */
- if ((adapter->flag & BOARD_64BIT) && (sizeof(dma_addr_t) == 8)) {
- pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
- adapter->has_64bit_addr = 1;
- } else {
- pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
- adapter->has_64bit_addr = 0;
- }
+ /* Set the Mode of addressing to 32 bit */
+ pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
+ adapter->has_64bit_addr = 0;
mutex_init(&adapter->int_mtx);
init_completion(&adapter->int_waitq);
With the driver now setup for default 32-bit DMA capabilities, remove the
broken make_local_pdev() implementation and usages.
Signed-off-by: Myron Stowe <[email protected]>
---
drivers/scsi/megaraid.c | 98 ++++++++---------------------------------------
1 files changed, 16 insertions(+), 82 deletions(-)
diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
index 32cca61..316924c 100644
--- a/drivers/scsi/megaraid.c
+++ b/drivers/scsi/megaraid.c
@@ -2023,29 +2023,6 @@ megaraid_abort_and_reset(adapter_t *adapter, Scsi_Cmnd *cmd, int aor)
return FALSE;
}
-static inline int
-make_local_pdev(adapter_t *adapter, struct pci_dev **pdev)
-{
- *pdev = alloc_pci_dev();
-
- if( *pdev == NULL ) return -1;
-
- memcpy(*pdev, adapter->dev, sizeof(struct pci_dev));
-
- if( pci_set_dma_mask(*pdev, DMA_BIT_MASK(32)) != 0 ) {
- kfree(*pdev);
- return -1;
- }
-
- return 0;
-}
-
-static inline void
-free_local_pdev(struct pci_dev *pdev)
-{
- kfree(pdev);
-}
-
/**
* mega_allocate_inquiry()
* @dma_handle - handle returned for dma address
@@ -2209,13 +2186,10 @@ proc_show_rebuild_rate(struct seq_file *m, void *v)
adapter_t *adapter = m->private;
dma_addr_t dma_handle;
caddr_t inquiry;
- struct pci_dev *pdev;
-
- if( make_local_pdev(adapter, &pdev) != 0 )
- return 0;
+ struct pci_dev *pdev = adapter->dev;
if( (inquiry = mega_allocate_inquiry(&dma_handle, pdev)) == NULL )
- goto free_pdev;
+ return 0;
if( mega_adapinq(adapter, dma_handle) != 0 ) {
seq_puts(m, "Adapter inquiry failed.\n");
@@ -2233,8 +2207,6 @@ proc_show_rebuild_rate(struct seq_file *m, void *v)
free_inquiry:
mega_free_inquiry(inquiry, dma_handle, pdev);
-free_pdev:
- free_local_pdev(pdev);
return 0;
}
@@ -2252,14 +2224,11 @@ proc_show_battery(struct seq_file *m, void *v)
adapter_t *adapter = m->private;
dma_addr_t dma_handle;
caddr_t inquiry;
- struct pci_dev *pdev;
+ struct pci_dev *pdev = adapter->dev;
u8 battery_status;
- if( make_local_pdev(adapter, &pdev) != 0 )
- return 0;
-
if( (inquiry = mega_allocate_inquiry(&dma_handle, pdev)) == NULL )
- goto free_pdev;
+ return 0;
if( mega_adapinq(adapter, dma_handle) != 0 ) {
seq_printf(m, "Adapter inquiry failed.\n");
@@ -2308,8 +2277,6 @@ proc_show_battery(struct seq_file *m, void *v)
free_inquiry:
mega_free_inquiry(inquiry, dma_handle, pdev);
-free_pdev:
- free_local_pdev(pdev);
return 0;
}
@@ -2357,18 +2324,15 @@ proc_show_pdrv(struct seq_file *m, adapter_t *adapter, int channel)
char *scsi_inq;
dma_addr_t scsi_inq_dma_handle;
caddr_t inquiry;
- struct pci_dev *pdev;
+ struct pci_dev *pdev = adapter->dev;
u8 *pdrv_state;
u8 state;
int tgt;
int max_channels;
int i;
- if( make_local_pdev(adapter, &pdev) != 0 )
- return 0;
-
if( (inquiry = mega_allocate_inquiry(&dma_handle, pdev)) == NULL )
- goto free_pdev;
+ return 0;
if( mega_adapinq(adapter, dma_handle) != 0 ) {
seq_puts(m, "Adapter inquiry failed.\n");
@@ -2453,8 +2417,6 @@ free_pci:
pci_free_consistent(pdev, 256, scsi_inq, scsi_inq_dma_handle);
free_inquiry:
mega_free_inquiry(inquiry, dma_handle, pdev);
-free_pdev:
- free_local_pdev(pdev);
return 0;
}
@@ -2533,17 +2495,14 @@ proc_show_rdrv(struct seq_file *m, adapter_t *adapter, int start, int end )
char *disk_array;
dma_addr_t disk_array_dma_handle;
caddr_t inquiry;
- struct pci_dev *pdev;
+ struct pci_dev *pdev = adapter->dev;
u8 *rdrv_state;
int num_ldrv;
u32 array_sz;
int i;
- if( make_local_pdev(adapter, &pdev) != 0 )
- return 0;
-
if( (inquiry = mega_allocate_inquiry(&dma_handle, pdev)) == NULL )
- goto free_pdev;
+ return 0;
if( mega_adapinq(adapter, dma_handle) != 0 ) {
seq_puts(m, "Adapter inquiry failed.\n");
@@ -2694,8 +2653,6 @@ free_pci:
disk_array_dma_handle);
free_inquiry:
mega_free_inquiry(inquiry, dma_handle, pdev);
-free_pdev:
- free_local_pdev(pdev);
return 0;
}
@@ -3164,6 +3121,7 @@ megadev_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
return (-ENODEV);
adapter = hba_soft_state[adapno];
+ pdev = adapter->dev;
/*
* Deletion of logical drive is a special case. The adapter
@@ -3208,12 +3166,10 @@ megadev_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
/*
* For all internal commands, the buffer must be allocated in
- * <4GB address range
+ * <4GB address range.
+ *
+ * Is it a passthru command or a DCMD
*/
- if( make_local_pdev(adapter, &pdev) != 0 )
- return -EIO;
-
- /* Is it a passthru command or a DCMD */
if( uioc.uioc_rmbox[0] == MEGA_MBOXCMD_PASSTHRU ) {
/* Passthru commands */
@@ -3221,10 +3177,8 @@ megadev_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
sizeof(mega_passthru),
&pthru_dma_hndl);
- if( pthru == NULL ) {
- free_local_pdev(pdev);
+ if( pthru == NULL )
return (-ENOMEM);
- }
/*
* The user passthru structure
@@ -3241,8 +3195,6 @@ megadev_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
sizeof(mega_passthru), pthru,
pthru_dma_hndl);
- free_local_pdev(pdev);
-
return (-EFAULT);
}
@@ -3260,8 +3212,6 @@ megadev_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
pthru,
pthru_dma_hndl);
- free_local_pdev(pdev);
-
return (-ENOMEM);
}
@@ -3331,8 +3281,6 @@ freemem_and_return:
pci_free_consistent(pdev, sizeof(mega_passthru),
pthru, pthru_dma_hndl);
- free_local_pdev(pdev);
-
return rval;
}
else {
@@ -3345,10 +3293,8 @@ freemem_and_return:
data = pci_alloc_consistent(pdev,
uioc.xferlen, &data_dma_hndl);
- if( data == NULL ) {
- free_local_pdev(pdev);
+ if( data == NULL )
return (-ENOMEM);
- }
uxferaddr = MBOX(uioc)->xferaddr;
}
@@ -3367,8 +3313,6 @@ freemem_and_return:
uioc.xferlen,
data, data_dma_hndl);
- free_local_pdev(pdev);
-
return (-EFAULT);
}
}
@@ -3391,8 +3335,6 @@ freemem_and_return:
data_dma_hndl);
}
- free_local_pdev(pdev);
-
return rval;
}
@@ -3413,8 +3355,6 @@ freemem_and_return:
data_dma_hndl);
}
- free_local_pdev(pdev);
-
return rval;
}
@@ -4068,22 +4008,18 @@ mega_internal_dev_inquiry(adapter_t *adapter, u8 ch, u8 tgt,
dma_addr_t pthru_dma_handle;
megacmd_t mc;
int rval;
- struct pci_dev *pdev;
+ struct pci_dev *pdev = adapter->dev;
/*
* For all internal commands, the buffer must be allocated in <4GB
* address range
*/
- if( make_local_pdev(adapter, &pdev) != 0 ) return -1;
-
pthru = pci_alloc_consistent(pdev, sizeof(mega_passthru),
&pthru_dma_handle);
- if( pthru == NULL ) {
- free_local_pdev(pdev);
+ if( pthru == NULL )
return -1;
- }
pthru->timeout = 2;
pthru->ars = 1;
@@ -4117,8 +4053,6 @@ mega_internal_dev_inquiry(adapter_t *adapter, u8 ch, u8 tgt,
pci_free_consistent(pdev, sizeof(mega_passthru), pthru,
pthru_dma_handle);
- free_local_pdev(pdev);
-
return rval;
}
#endif
With the driver now setup for default 32-bit DMA capabilities, remove the
64-bit DMA related dead code.
Signed-off-by: Myron Stowe <[email protected]>
---
drivers/scsi/megaraid.c | 45 +++++++++------------------------------------
drivers/scsi/megaraid.h | 1 -
2 files changed, 9 insertions(+), 37 deletions(-)
diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
index 316924c..58953ab 100644
--- a/drivers/scsi/megaraid.c
+++ b/drivers/scsi/megaraid.c
@@ -713,12 +713,7 @@ mega_build_cmd(adapter_t *adapter, Scsi_Cmnd *cmd, int *busy)
pthru->cdblen = cmd->cmd_len;
memcpy(pthru->cdb, cmd->cmnd, cmd->cmd_len);
- if( adapter->has_64bit_addr ) {
- mbox->m_out.cmd = MEGA_MBOXCMD_PASSTHRU64;
- }
- else {
- mbox->m_out.cmd = MEGA_MBOXCMD_PASSTHRU;
- }
+ mbox->m_out.cmd = MEGA_MBOXCMD_PASSTHRU;
scb->dma_direction = PCI_DMA_FROMDEVICE;
@@ -750,16 +745,9 @@ mega_build_cmd(adapter_t *adapter, Scsi_Cmnd *cmd, int *busy)
* A little hack: 2nd bit is zero for all scsi read
* commands and is set for all scsi write commands
*/
- if( adapter->has_64bit_addr ) {
- mbox->m_out.cmd = (*cmd->cmnd & 0x02) ?
- MEGA_MBOXCMD_LWRITE64:
- MEGA_MBOXCMD_LREAD64 ;
- }
- else {
- mbox->m_out.cmd = (*cmd->cmnd & 0x02) ?
- MEGA_MBOXCMD_LWRITE:
- MEGA_MBOXCMD_LREAD ;
- }
+ mbox->m_out.cmd = (*cmd->cmnd & 0x02) ?
+ MEGA_MBOXCMD_LWRITE:
+ MEGA_MBOXCMD_LREAD ;
/*
* 6-byte READ(0x08) or WRITE(0x0A) cdb
@@ -929,13 +917,7 @@ mega_build_cmd(adapter_t *adapter, Scsi_Cmnd *cmd, int *busy)
channel, target);
/* Initialize mailbox */
- if( adapter->has_64bit_addr ) {
- mbox->m_out.cmd = MEGA_MBOXCMD_PASSTHRU64;
- }
- else {
- mbox->m_out.cmd = MEGA_MBOXCMD_PASSTHRU;
- }
-
+ mbox->m_out.cmd = MEGA_MBOXCMD_PASSTHRU;
mbox->m_out.xferaddr = scb->pthru_dma_addr;
}
@@ -1763,7 +1745,7 @@ mega_build_sglist(adapter_t *adapter, scb_t *scb, u32 *buf, u32 *len)
*len = 0;
- if (scsi_sg_count(cmd) == 1 && !adapter->has_64bit_addr) {
+ if (scsi_sg_count(cmd) == 1) {
sg = scsi_sglist(cmd);
scb->dma_h_bulkdata = sg_dma_address(sg);
*buf = (u32)scb->dma_h_bulkdata;
@@ -1772,13 +1754,8 @@ mega_build_sglist(adapter_t *adapter, scb_t *scb, u32 *buf, u32 *len)
}
scsi_for_each_sg(cmd, sg, sgcnt, idx) {
- if (adapter->has_64bit_addr) {
- scb->sgl64[idx].address = sg_dma_address(sg);
- *len += scb->sgl64[idx].length = sg_dma_len(sg);
- } else {
- scb->sgl[idx].address = sg_dma_address(sg);
- *len += scb->sgl[idx].length = sg_dma_len(sg);
- }
+ scb->sgl[idx].address = sg_dma_address(sg);
+ *len += scb->sgl[idx].length = sg_dma_len(sg);
}
/* Reset pointer and length fields */
@@ -2076,10 +2053,7 @@ proc_show_config(struct seq_file *m, void *v)
if(adapter->flag & BOARD_64BIT)
seq_puts(m, "Controller capable of 64-bit memory addressing\n");
- if( adapter->has_64bit_addr )
- seq_puts(m, "Controller using 64-bit memory addressing\n");
- else
- seq_puts(m, "Controller is not using 64-bit memory addressing\n");
+ seq_puts(m, "Controller is not using 64-bit memory addressing\n");
seq_printf(m, "Base = %08lx, Irq = %d, ",
adapter->base, adapter->host->irq);
@@ -4471,7 +4445,6 @@ megaraid_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
/* Set the Mode of addressing to 32 bit */
pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
- adapter->has_64bit_addr = 0;
mutex_init(&adapter->int_mtx);
init_completion(&adapter->int_waitq);
diff --git a/drivers/scsi/megaraid.h b/drivers/scsi/megaraid.h
index 4d0ce4e..17e449b 100644
--- a/drivers/scsi/megaraid.h
+++ b/drivers/scsi/megaraid.h
@@ -827,7 +827,6 @@ typedef struct {
#endif
- int has_64bit_addr; /* are we using 64-bit addressing */
int support_ext_cdb;
int boot_ldrv_enabled;
int boot_ldrv;
On Tue, 2013-07-09 at 14:39 -0600, Myron Stowe wrote:
> Is the "megaraid" driver still actively used and maintained? I originally
> posted this series on 06.07.2013 and after receiving no comments, pinged
> the list again on 06.17.2013 and still received no comments/feedback.
>
> Trying again as I believe there is a real issue here, which I'd like
> confirmation on, and we really should remove the local copy/usage of
> 'struct pci_dev' that this driver currently maintains.
>
>
> While the megaraid device itself may be 64-bit DMA capable, 32-bit address
> restricted DMA buffers are apparently required for "internal commands" as
> is denoted by a couple of comments - "For all internal commands, the
> buffer must be allocated in <4GB address range" - within the driver.
>
> If the device is 64-bit DMA capable then, once it is setup, any subsequent
> DMA allocations for "internal commands" would not be properly restricted
> due to megaraid_probe_one() having called pci_set_dma_mask() on pdev with
> DMA_BIT_MASK(64). The driver attempts to solve this by using
> make_local_pdev() to dynamically create local pci_dev structures which are
> then set and used for allocating 32-bit address space restricted DMA
> buffers[1] but I don't believe that the implementation works as intended.
>
>
> Assume that the megaraid device is 64-bit DMA capable. While probing the
> device and attaching the megaraid driver, pci_set_dma_mask() is called
> with the "originating pdev" and a DMA_BIT_MASK of 64. As a result, any
> subsequent dynamic DMA related allocations associated with the
> "originating pdev" will acquire 64-bit based buffers, which do not meet
> the addressing restrictions for internal commands.
>
> megaraid_probe_one(struct pci_dev *pdev, ...)
> ...
> pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
>
> As mentioned, the driver attempts to solve this by using make_local_pdev()
> to dynamically create local pci_dev structures - "local pdev's" - which
> are set with a DMA_BIT_MASK of 32.
>
> make_local_pdev
> alloc_pci_dev
> memcpy
> pci_set_dma_mask
> dma_set_mask
> *dev->dma_mask = mask;
>
> The "local pdev" is then used in allocating a DMA buffer in an attempt to
> meet the < 4 GB restriction.
>
> For a 64-bit DMA capable device, the "originating pdev" will have its
> 'dma_mask' set to 0xffffffffffffffff after the driver attaches.
> Subsequently, when an internal command is initiated, make_local_pdev() is
> called. make_local_pdev() uses the PCI's core to allocate a "local pdev"
> and then copies the "originating pdev" content into the newly allocated
> "local pdev". As a result of copying the "originating pdev" content into
> the "local pdev", pdev->dev.dma_mask will be pointing back to the
> "originating pdev's" 'dma_mask' member, not the "local pdev's" as
> intended. Thus, when make_local_pdev() calls pci_set_dma_mask() in an
> attempt to set the "local pdev's" DMA mask to 32 it will instead overwrite
> the "originating pdev's" DMA mask. Thus, after any user initiated
> commands are issued, all subsequent DMA allocations will be 32-bit
> restricted from that point onward regardless of whether they are internal
> commands or otherwise.
>
>
> This patch fixes the issue by removing the setup of DMA_BIT_MASK to 64 in
> megaraid_probe_one(), leaving the driver with default 32-bit DMA
> capabilities, as it currently ends up in such a state anyway after any
> internal commands are initiated.
>
>
> [1] It seems strange that both mega_buffer/buf_dma_handle and
> make_local_pdev() both exist for internal commands but this has been
> the case for a long time - at least since 2.6.12-rc2. Perhaps there
> is some coalescing that could be done.
> ---
> Myron Stowe (3):
> [SCSI] megaraid: Remove 64-bit DMA related dead code
> [SCSI] megaraid: Remove local pdev's
> [SCSI] megaraid: Remove 64-bit DMA_BIT_MASK capability
Adam, you do drive by coding on this for LSI ... ack or reject, please.
James
On Tue, Jul 9, 2013 at 2:18 PM, James Bottomley
<[email protected]> wrote:
> On Tue, 2013-07-09 at 14:39 -0600, Myron Stowe wrote:
>> Is the "megaraid" driver still actively used and maintained? I originally
>> posted this series on 06.07.2013 and after receiving no comments, pinged
>> the list again on 06.17.2013 and still received no comments/feedback.
>>
>> Trying again as I believe there is a real issue here, which I'd like
>> confirmation on, and we really should remove the local copy/usage of
>> 'struct pci_dev' that this driver currently maintains.
>>
>>
>> While the megaraid device itself may be 64-bit DMA capable, 32-bit address
>> restricted DMA buffers are apparently required for "internal commands" as
>> is denoted by a couple of comments - "For all internal commands, the
>> buffer must be allocated in <4GB address range" - within the driver.
>>
>> If the device is 64-bit DMA capable then, once it is setup, any subsequent
>> DMA allocations for "internal commands" would not be properly restricted
>> due to megaraid_probe_one() having called pci_set_dma_mask() on pdev with
>> DMA_BIT_MASK(64). The driver attempts to solve this by using
>> make_local_pdev() to dynamically create local pci_dev structures which are
>> then set and used for allocating 32-bit address space restricted DMA
>> buffers[1] but I don't believe that the implementation works as intended.
>>
>>
>> Assume that the megaraid device is 64-bit DMA capable. While probing the
>> device and attaching the megaraid driver, pci_set_dma_mask() is called
>> with the "originating pdev" and a DMA_BIT_MASK of 64. As a result, any
>> subsequent dynamic DMA related allocations associated with the
>> "originating pdev" will acquire 64-bit based buffers, which do not meet
>> the addressing restrictions for internal commands.
>>
>> megaraid_probe_one(struct pci_dev *pdev, ...)
>> ...
>> pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
>>
>> As mentioned, the driver attempts to solve this by using make_local_pdev()
>> to dynamically create local pci_dev structures - "local pdev's" - which
>> are set with a DMA_BIT_MASK of 32.
>>
>> make_local_pdev
>> alloc_pci_dev
>> memcpy
>> pci_set_dma_mask
>> dma_set_mask
>> *dev->dma_mask = mask;
>>
>> The "local pdev" is then used in allocating a DMA buffer in an attempt to
>> meet the < 4 GB restriction.
>>
>> For a 64-bit DMA capable device, the "originating pdev" will have its
>> 'dma_mask' set to 0xffffffffffffffff after the driver attaches.
>> Subsequently, when an internal command is initiated, make_local_pdev() is
>> called. make_local_pdev() uses the PCI's core to allocate a "local pdev"
>> and then copies the "originating pdev" content into the newly allocated
>> "local pdev". As a result of copying the "originating pdev" content into
>> the "local pdev", pdev->dev.dma_mask will be pointing back to the
>> "originating pdev's" 'dma_mask' member, not the "local pdev's" as
>> intended. Thus, when make_local_pdev() calls pci_set_dma_mask() in an
>> attempt to set the "local pdev's" DMA mask to 32 it will instead overwrite
>> the "originating pdev's" DMA mask. Thus, after any user initiated
>> commands are issued, all subsequent DMA allocations will be 32-bit
>> restricted from that point onward regardless of whether they are internal
>> commands or otherwise.
>>
>>
>> This patch fixes the issue by removing the setup of DMA_BIT_MASK to 64 in
>> megaraid_probe_one(), leaving the driver with default 32-bit DMA
>> capabilities, as it currently ends up in such a state anyway after any
>> internal commands are initiated.
>>
>>
>> [1] It seems strange that both mega_buffer/buf_dma_handle and
>> make_local_pdev() both exist for internal commands but this has been
>> the case for a long time - at least since 2.6.12-rc2. Perhaps there
>> is some coalescing that could be done.
>> ---
>> Myron Stowe (3):
>> [SCSI] megaraid: Remove 64-bit DMA related dead code
>> [SCSI] megaraid: Remove local pdev's
>> [SCSI] megaraid: Remove 64-bit DMA_BIT_MASK capability
>
> Adam, you do drive by coding on this for LSI ... ack or reject, please.
>
> James
>
>
James,
I have just now located my box of MegaRAID Parallel SCSI controllers.
I will review and test the patch series from Myron and respond by next Monday.
-Adam
On Tue, 2013-07-09 at 15:12 -0700, adam radford wrote:
> On Tue, Jul 9, 2013 at 2:18 PM, James Bottomley
> > Adam, you do drive by coding on this for LSI ... ack or reject, please.
> I have just now located my box of MegaRAID Parallel SCSI controllers.
> I will review and test the patch series from Myron and respond by next Monday.
Thanks,
James
On Tue, Jul 9, 2013 at 11:10 PM, James Bottomley
<[email protected]> wrote:
> On Tue, 2013-07-09 at 15:12 -0700, adam radford wrote:
>> On Tue, Jul 9, 2013 at 2:18 PM, James Bottomley
>> > Adam, you do drive by coding on this for LSI ... ack or reject, please.
>
>> I have just now located my box of MegaRAID Parallel SCSI controllers.
>> I will review and test the patch series from Myron and respond by next Monday.
>
> Thanks,
>
> James
>
>
Unfortunately my box of MegaRAID Parallel SCSI controllers only
contains only cards intended for megaraid_mbox.c (I tested all 20 of
them),
and does not contain any of the following really old Symbios based
megaraid cards:
static struct pci_device_id megaraid_pci_tbl[] = {
{PCI_VENDOR_ID_AMI, PCI_DEVICE_ID_AMI_MEGARAID,
PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
{PCI_VENDOR_ID_AMI, PCI_DEVICE_ID_AMI_MEGARAID2,
PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_AMI_MEGARAID3,
PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
{0,}
};
which I had located previously before the company headquarters moved.
I cannot currently locate any of the above 3 controllers anywhere at
LSI headquarters after an exhaustive search, so I cannot test the
patches to megaraid.c from Myron @ RedHat.
Myron, do you actually have the hardware and have you tested the
patches yourself ?
-Adam
Well??? I responded yesterday but I still don't see it on the list so
at the expense of some getting a double response I'll try again ...
On Mon, Jul 22, 2013 at 6:27 PM, adam radford <[email protected]> wrote:
> On Tue, Jul 9, 2013 at 11:10 PM, James Bottomley
> <[email protected]> wrote:
>> On Tue, 2013-07-09 at 15:12 -0700, adam radford wrote:
>>> On Tue, Jul 9, 2013 at 2:18 PM, James Bottomley
>>> > Adam, you do drive by coding on this for LSI ... ack or reject, please.
>>
>>> I have just now located my box of MegaRAID Parallel SCSI controllers.
>>> I will review and test the patch series from Myron and respond by next Monday.
>>
>> Thanks,
>>
>> James
>>
>>
>
> Unfortunately my box of MegaRAID Parallel SCSI controllers only
> contains only cards intended for megaraid_mbox.c (I tested all 20 of
> them),
>
> and does not contain any of the following really old Symbios based
> megaraid cards:
>
> static struct pci_device_id megaraid_pci_tbl[] = {
> {PCI_VENDOR_ID_AMI, PCI_DEVICE_ID_AMI_MEGARAID,
> PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
> {PCI_VENDOR_ID_AMI, PCI_DEVICE_ID_AMI_MEGARAID2,
> PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
> {PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_AMI_MEGARAID3,
> PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
> {0,}
> };
>
> which I had located previously before the company headquarters moved.
> I cannot currently locate any of the above 3 controllers anywhere at
> LSI headquarters after an exhaustive search, so I cannot test the
> patches to megaraid.c from Myron @ RedHat.
Adam:
Thanks for looking/trying.
>
> Myron, do you actually have the hardware and have you tested the
> patches yourself ?
No, the closest to the above that I have access to is a NEC platform
with a PCI_VENDOR_ID_NCR vendor's PCI_DEVICE_ID_AMI_MEGARAID3 device.
I stumbled onto the issue because I want to get rid of the driver's use
of 'struct pci_dev'. It seems, from my looking, to be the only driver
that keeps local copies of such, and upon further investigation I
noticed that the intended use of the local copies is flawed to the point
that keeping them makes no sense (which further supported my desire to
get rid of them).
Myron
>
> -Adam
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html