RE: [PATCH v2 0/10] megaraid_sas : Updates for scsi for-next
> -Original Message- > From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] > Sent: Saturday, August 29, 2015 1:03 AM > To: sumit.sax...@avagotech.com > Cc: linux-scsi@vger.kernel.org; the...@redhat.com; > martin.peter...@oracle.com; h...@infradead.org; jbottom...@parallels.com; > kashyap.de...@avagotech.com; kiran-kumar.kast...@avagotech.com; > uday.ling...@avagotech.com; Bjorn Helgaas > Subject: Re: [PATCH v2 0/10] megaraid_sas : Updates for scsi for-next > > On Thu, 2015-08-20 at 18:43 +0530, sumit.sax...@avagotech.com wrote: > > MegaRaid driver changes. This patch set is resent based on feedback > > received > by Martin Petersen. > > Please consider this patch set for next kernel release. > > > > Signed-off-by: Sumit Saxena > > Signed-off-by: Kashyap Desai > > --- > > [PATCH v2 00/10] megaraid_sas : Updates for scsi for-next. > > [PATCH v2 01/10] megaraid_sas : Synchronize driver headers with firmware > APIs. > > [PATCH v2 02/10] megaraid_sas : Increase timeout to 60 secs for abort > > frames > during shutdown. > > [PATCH v2 03/10] megaraid_sas : Jbod sequence number support. > > [PATCH v2 04/10] megaraid_sas : Code cleanup-use local variable drv_ops > inside megasas_ioc_init_fusion. > > [PATCH v2 05/10] megaraid_sas : Support for max_io_size 1MB. > > [PATCH v2 06/10] megaraid_sas : Chip reset if driver fail to bring ioc > > ready. > > [PATCH v2 07/10] megaraid_sas : Print critical fw event message. > > [PATCH v2 08/10] megaraid_sas : Fix validHandles check in io path. > > [PATCH v2 09/10] megaraid_sas : Code refactor for use of requestorId. > > [PATCH v2 10/10] megaraid_sas : Version upgrade. > > I'm getting a ton of rejects in this series because of > > commit da0dc9fb4e6b0ad5a947c27a3c48985f6a2377eb > Author: Bjorn Helgaas > Date: Tue Jul 7 15:52:45 2015 -0500 > > megaraid_sas: fix whitespace errors > > Please fix. > > Useful rules of thumb are don't ack whitespace patches because they > inevitably > cause this type of issue. If you do decide to ack, make sure you don't > cause > conflicts with your own outstanding patches. > > James James, I have sent patches rebased on latest code few days back. Can you provide your feedback? Thanks, Sumit > > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 09/30] cxlflash: Fix to stop interrupt processing on remove
On 17.9.2015 19:16, Matthew R. Ochs wrote:
>> On Sep 17, 2015, at 7:38 AM, Tomas Henzl wrote:
>>
>> On 16.9.2015 18:53, Matthew R. Ochs wrote:
>>> Interrupt processing can run in parallel to a remove operation. This
>>> can lead to a condition where the interrupt handler is processing with
>>> memory that has been freed.
>>>
>>> To avoid processing an interrupt while memory may be yanked, check for
>>> removal while in the interrupt handler. Bail when removal is imminent.
>>>
>>> Signed-off-by: Matthew R. Ochs
>>> Signed-off-by: Manoj N. Kumar
>>> ---
>>> drivers/scsi/cxlflash/common.h | 2 ++
>>> drivers/scsi/cxlflash/main.c | 21 +++--
>>> 2 files changed, 17 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/scsi/cxlflash/common.h b/drivers/scsi/cxlflash/common.h
>>> index 1abe4e0..03d2cc6 100644
>>> --- a/drivers/scsi/cxlflash/common.h
>>> +++ b/drivers/scsi/cxlflash/common.h
>>> @@ -103,6 +103,8 @@ struct cxlflash_cfg {
>>> enum cxlflash_lr_state lr_state;
>>> int lr_port;
>>>
>>> + atomic_t remove_active;
>>> +
>>> struct cxl_afu *cxl_afu;
>>>
>>> struct pci_pool *cxlflash_cmd_pool;
>>> diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c
>>> index 6e85c77..89ee648 100644
>>> --- a/drivers/scsi/cxlflash/main.c
>>> +++ b/drivers/scsi/cxlflash/main.c
>>> @@ -892,6 +892,7 @@ static void cxlflash_remove(struct pci_dev *pdev)
>>> spin_unlock_irqrestore(&cfg->tmf_waitq.lock, lock_flags);
>>>
>>> cfg->state = STATE_FAILTERM;
>>> + atomic_inc(&cfg->remove_active);
>> Hi Matthew,
>> you could just call term_afu at this point, this way you don't
>> need an additional check in all irq functions.
>> Cheers,
>> Tomas
> Hi Tomas,
>
> We actually do call term_afu() a few lines down from here. I don't follow
> how moving it here would help things.
When you disable ints sooner (that is what term_afu does ?) you'll get no
more ints later isn't this what you want?
>
> The reason for the atomic was to provide something lightweight that we
> could check _inside_ the processing loop for the read-response queue
> handler. A check outside that loop doesn't really provide much in terms
> of closing or narrowing down the window of when freed memory can be
> accessed.
>
> As David Laight correctly pointed out, this approach does not completely
> close the window. We'd need something heavier to fully protect (e.g. a lock
> to wrap around the entire loop). I will look into adding this in a future
> cycle
> when I can adequately test.
term_afu calls free_irq and this function
does not return until any executing interrupts for have completed.
This is the sync mechanism you need, it's lightweight
(does not add an additional check to your irq functions)
and closes the race window completely.
--tm
>
>
> -matt
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 08/30] cxlflash: Fix to avoid CXL services during EEH
On 09/16/2015 04:27 PM, Matthew R. Ochs wrote:
> --- a/drivers/scsi/cxlflash/main.c
> +++ b/drivers/scsi/cxlflash/main.c
> @@ -2311,6 +2311,7 @@ static int cxlflash_probe(struct pci_dev *pdev,
> cfg->lr_port = -1;
> mutex_init(&cfg->ctx_tbl_list_mutex);
> mutex_init(&cfg->ctx_recovery_mutex);
> + init_rwsem(&cfg->ioctl_rwsem);
> INIT_LIST_HEAD(&cfg->ctx_err_recovery);
> INIT_LIST_HEAD(&cfg->lluns);
>
> @@ -2365,6 +2366,19 @@ out_remove:
> }
>
> /**
> + * drain_ioctls() - wait until all currently executing ioctls have completed
> + * @cfg: Internal structure associated with the host.
> + *
> + * Obtain write access to read/write semaphore that wraps ioctl
> + * handling to 'drain' ioctls currently executing.
> + */
> +static void drain_ioctls(struct cxlflash_cfg *cfg)
> +{
> + down_write(&cfg->ioctl_rwsem);
> + up_write(&cfg->ioctl_rwsem);
> +}
> +
> +/**
> * cxlflash_pci_error_detected() - called when a PCI error is detected
> * @pdev:PCI device struct.
> * @state: PCI channel state.
> @@ -2383,16 +2397,14 @@ static pci_ers_result_t
> cxlflash_pci_error_detected(struct pci_dev *pdev,
> switch (state) {
> case pci_channel_io_frozen:
> cfg->state = STATE_LIMBO;
> -
> - /* Turn off legacy I/O */
> scsi_block_requests(cfg->host);
> + drain_ioctls(cfg);
So, what kicks any outstanding ioctls back? Let's assume you are in the middle
of disk_attach
and you've sent the READ_CAP16 to the device. It appears as if what would
happen here is we'd
sit here in cxlflash_pci_error_detected. Eventually, the READ_CAP16 would
timeout. This would
wake the SCSI error handler, and end up calling your eh_device_reset handler,
which would see that
we are in STATE_LIMBO, where it would then do a wait_event, waiting for us to
get out of STATE_LIMBO,
and we would end up in a deadlock.
Rather than implementing a rw semaphore, would it be better to simply make the
ioctls check the
state we are in and either wait to get out of EEH state or fail themselves?
> rc = cxlflash_mark_contexts_error(cfg);
> if (unlikely(rc))
> dev_err(dev, "%s: Failed to mark user contexts!(%d)\n",
> __func__, rc);
> term_mc(cfg, UNDO_START);
> stop_afu(cfg);
> -
> return PCI_ERS_RESULT_NEED_RESET;
> case pci_channel_io_perm_failure:
> cfg->state = STATE_FAILTERM;
> diff --git a/drivers/scsi/cxlflash/superpipe.c
> b/drivers/scsi/cxlflash/superpipe.c
> index cf2a85d..8a18230 100644
> --- a/drivers/scsi/cxlflash/superpipe.c
> +++ b/drivers/scsi/cxlflash/superpipe.c
> @@ -1214,6 +1214,48 @@ static const struct file_operations null_fops = {
> };
>
> /**
> + * check_state() - checks and responds to the current adapter state
> + * @cfg: Internal structure associated with the host.
> + * @ioctl: Indicates if on an ioctl thread.
> + *
> + * This routine can block and should only be used on process context.
> + * When blocking on an ioctl thread, the ioctl read semaphore should be
> + * let up to allow for draining actively running ioctls. Also note that
> + * when waking up from waiting in reset, the state is unknown and must
> + * be checked again before proceeding.
> + *
> + * Return: 0 on success, -errno on failure
> + */
> +static int check_state(struct cxlflash_cfg *cfg, bool ioctl)
All your callers appear to set the second parameter to true, so why bother
having it?
> +{
> + struct device *dev = &cfg->dev->dev;
> + int rc = 0;
> +
> +retry:
> + switch (cfg->state) {
> + case STATE_LIMBO:
> + dev_dbg(dev, "%s: Limbo state, going to wait...\n", __func__);
> + if (ioctl)
> + up_read(&cfg->ioctl_rwsem);
> + rc = wait_event_interruptible(cfg->limbo_waitq,
> + cfg->state != STATE_LIMBO);
> + if (ioctl)
> + down_read(&cfg->ioctl_rwsem);
> + if (unlikely(rc))
> + break;
> + goto retry;
> + case STATE_FAILTERM:
> + dev_dbg(dev, "%s: Failed/Terminating!\n", __func__);
> + rc = -ENODEV;
> + break;
> + default:
> + break;
> + }
> +
> + return rc;
> +}
> +
> +/**
> * cxlflash_disk_attach() - attach a LUN to a context
> * @sdev:SCSI device associated with LUN.
> * @attach: Attach ioctl data structure.
--
Brian King
Power Linux I/O
IBM Linux Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bugfix 3/3] eata: Enhance eata driver to support PCI device hot-removal
Christoph Hellwig wrote on 16/09/15 23:12: Jiang, you also need to convert the driver to scsi_add_host/scsi_remove_host from the legacy scsi_register interface, otherwise the SCSI layer will be very unhappy. Take a look at commit 0d31f8759109cbc1e6fc196d08e6b0e8a9e93b3f for example, the change should be straight forward. I am pleased to note that when I tried a Linus git head kernel from the last 24 hours, the IRQ routing for my DPT2044W SCSI card using eata module worked again, although the shut-down/kexec issue remains. Arthur. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 10/30] cxlflash: Correct naming of limbo state and waitq
Reviewed-by: Brian King -- Brian King Power Linux I/O IBM Linux Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 11/30] cxlflash: Make functions static
Reviewed-by: Brian King -- Brian King Power Linux I/O IBM Linux Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/17] Add ida and idr helper routines.
On 09/15/2015 11:41 AM, Tejun Heo wrote: > Hello, > > On Tue, Sep 15, 2015 at 11:38:42AM -0700, James Bottomley wrote: >> For most of the SCSI stuff, yes. I'm less sure about the sd numbers. >> They go up very high and get hammered a lot during system bring up and >> hot plug. I think having their own lock rather than wrapping everything >> around simple_ida_lock makes more sense here just because the system is >> heavily contended on getting indexes at bring up. >> >> To continue the thought, why not move simple_ida_lock into struct ida so >> we don't have to worry about the contention and can sue ida_simple_... >> everywhere? > > We sure can do that if necessary but I'm rather doubtful that even > with sd number hammering this is likely to be a problem. Let's > convert the users to the simple interface and make the lock per-ida if > we actually see contention on the lock. > > Thanks. > To be clear: you would like a patch series that converts the users of the ida_* routines in my patches to instead use the ida_simple_* routines, correct? And of course the ida_* helper routines I was adding in idr.h would not be needed. If this is correct, I will supply a version 2 patch series that addresses this issue as well as the two patch-naming issues that were raised. -- Lee Duncan -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/17] Add ida and idr helper routines.
Hello, On Fri, Sep 18, 2015 at 08:42:26AM -0700, Lee Duncan wrote: > To be clear: you would like a patch series that converts the users of > the ida_* routines in my patches to instead use the ida_simple_* > routines, correct? And of course the ida_* helper routines I was adding > in idr.h would not be needed. Yeap. > If this is correct, I will supply a version 2 patch series that > addresses this issue as well as the two patch-naming issues that were > raised. Sounds good to me. If the shared lock blows up, we can make that per-ida. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] sci: ufs: Fix module autoload for OF platform driver
This platform driver has a OF device ID table but the OF module
alias information is not created so module autoloading won't work.
Signed-off-by: Luis de Bethencourt
---
Hello,
This patch adds the missing MODULE_DEVICE_TABLE() for OF to export
that information so modules have the correct aliases built-in and
autoloading works correctly.
A longer explanation by Javier Canillas can be found here:
https://lkml.org/lkml/2015/7/30/519
Thanks,
Luis
drivers/scsi/ufs/ufshcd-pltfrm.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
index 7db9564..7f3b0cf 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -375,6 +375,7 @@ static const struct of_device_id ufs_of_match[] = {
{ .compatible = "jedec,ufs-1.1"},
{},
};
+MODULE_DEVICE_TABLE(of, ufs_of_match);
static const struct dev_pm_ops ufshcd_dev_pm_ops = {
.suspend= ufshcd_pltfrm_suspend,
--
2.4.6
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 12/30] cxlflash: Refine host/device attributes
On 09/16/2015 04:29 PM, Matthew R. Ochs wrote:
> Implement the following suggestions and add two new attributes
> to allow for debugging the port LUN table.
>
> - use scnprintf() instead of snprintf()
> - use DEVICE_ATTR_RO and DEVICE_ATTR_RW
>
> Signed-off-by: Matthew R. Ochs
> Signed-off-by: Manoj N. Kumar
> Suggested-by: Shane Seymour
> ---
> drivers/scsi/cxlflash/main.c | 180
> +--
> 1 file changed, 138 insertions(+), 42 deletions(-)
>
> /**
> - * cxlflash_show_dev_mode() - presents the current mode of the device
> + * cxlflash_show_port_lun_table() - queries and presents the port LUN table
> + * @port:Desired port for status reporting.
> + * @afu: AFU owning the specified port.
> + * @buf: Buffer of length PAGE_SIZE to report back port status in ASCII.
> + *
> + * Return: The size of the ASCII string returned in @buf.
> + */
> +static ssize_t cxlflash_show_port_lun_table(u32 port,
> + struct afu *afu,
> + char *buf)
> +{
> + int i;
> + ssize_t bytes = 0;
> + __be64 __iomem *fc_port;
> +
> + if (port >= NUM_FC_PORTS)
> + return 0;
> +
> + fc_port = &afu->afu_map->global.fc_port[port][0];
> +
> + for (i = 0; i < CXLFLASH_NUM_VLUNS; i++, buf += 22)
Rather than this bug prone hard coded 22, how about never incrementing buf and
do something
similar to this:
> + bytes += scnprintf(buf, PAGE_SIZE, "%03d: %016llX\n",
> +i, readq_be(&fc_port[i]));
bytes += scnprintf(&buf[bytes], PAGE_SIZE, "%03d: %016llX\n",
i, readq_be(&fc_port[i]));
> + return bytes;
> +}
> +
--
Brian King
Power Linux I/O
IBM Linux Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 13/30] cxlflash: Fix to avoid spamming the kernel log
Reviewed-by: Brian King -- Brian King Power Linux I/O IBM Linux Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 04/30] cxlflash: Fix potential oops following LUN removal
> On Sep 17, 2015, at 8:26 PM, Brian King wrote:
>
> On 09/16/2015 04:27 PM, Matthew R. Ochs wrote:
>>
>> lun_access = kzalloc(sizeof(*lun_access), GFP_KERNEL);
>> if (unlikely(!lun_access)) {
>> dev_err(dev, "%s: Unable to allocate lun_access!\n", __func__);
>> +scsi_device_put(sdev);
>
> Looks like you've got a double scsi_device_put in this path, since there is
> another put
> in the the err0 path.
Good catch! I'll fix in v3.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 09/30] cxlflash: Fix to stop interrupt processing on remove
> On Sep 18, 2015, at 6:59 AM, Tomas Henzl wrote:
> On 17.9.2015 19:16, Matthew R. Ochs wrote:
>>> On Sep 17, 2015, at 7:38 AM, Tomas Henzl wrote:
>>>
>>> On 16.9.2015 18:53, Matthew R. Ochs wrote:
Interrupt processing can run in parallel to a remove operation. This
can lead to a condition where the interrupt handler is processing with
memory that has been freed.
To avoid processing an interrupt while memory may be yanked, check for
removal while in the interrupt handler. Bail when removal is imminent.
Signed-off-by: Matthew R. Ochs
Signed-off-by: Manoj N. Kumar
---
drivers/scsi/cxlflash/common.h | 2 ++
drivers/scsi/cxlflash/main.c | 21 +++--
2 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/cxlflash/common.h
b/drivers/scsi/cxlflash/common.h
index 1abe4e0..03d2cc6 100644
--- a/drivers/scsi/cxlflash/common.h
+++ b/drivers/scsi/cxlflash/common.h
@@ -103,6 +103,8 @@ struct cxlflash_cfg {
enum cxlflash_lr_state lr_state;
int lr_port;
+ atomic_t remove_active;
+
struct cxl_afu *cxl_afu;
struct pci_pool *cxlflash_cmd_pool;
diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c
index 6e85c77..89ee648 100644
--- a/drivers/scsi/cxlflash/main.c
+++ b/drivers/scsi/cxlflash/main.c
@@ -892,6 +892,7 @@ static void cxlflash_remove(struct pci_dev *pdev)
spin_unlock_irqrestore(&cfg->tmf_waitq.lock, lock_flags);
cfg->state = STATE_FAILTERM;
+ atomic_inc(&cfg->remove_active);
>>> Hi Matthew,
>>> you could just call term_afu at this point, this way you don't
>>> need an additional check in all irq functions.
>>> Cheers,
>>> Tomas
>> Hi Tomas,
>>
>> We actually do call term_afu() a few lines down from here. I don't follow
>> how moving it here would help things.
>
> When you disable ints sooner (that is what term_afu does ?) you'll get no
> more ints later isn't this what you want?
Correct, that's what we want.
>> The reason for the atomic was to provide something lightweight that we
>> could check _inside_ the processing loop for the read-response queue
>> handler. A check outside that loop doesn't really provide much in terms
>> of closing or narrowing down the window of when freed memory can be
>> accessed.
>>
>> As David Laight correctly pointed out, this approach does not completely
>> close the window. We'd need something heavier to fully protect (e.g. a lock
>> to wrap around the entire loop). I will look into adding this in a future
>> cycle
>> when I can adequately test.
>
> term_afu calls free_irq and this function
> does not return until any executing interrupts for have completed.
> This is the sync mechanism you need, it's lightweight
> (does not add an additional check to your irq functions)
> and closes the race window completely.
Thanks for clarifying!
I looked at this closer and you are correct, free_irq() guarantees not
to return until the interrupt handler has completed. The current location
of term_afu() is appropriate as the memory that the handler touches is
not freed until the very end [by free_mem() and scsi_host_put()] of the
remove. Thus we can simply ignore this patch (I'll remove it in a v3).
-matt
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 08/30] cxlflash: Fix to avoid CXL services during EEH
> On Sep 18, 2015, at 8:37 AM, Brian King wrote:
> On 09/16/2015 04:27 PM, Matthew R. Ochs wrote:
>>
>> /**
>> + * drain_ioctls() - wait until all currently executing ioctls have completed
>> + * @cfg:Internal structure associated with the host.
>> + *
>> + * Obtain write access to read/write semaphore that wraps ioctl
>> + * handling to 'drain' ioctls currently executing.
>> + */
>> +static void drain_ioctls(struct cxlflash_cfg *cfg)
>> +{
>> +down_write(&cfg->ioctl_rwsem);
>> +up_write(&cfg->ioctl_rwsem);
>> +}
>> +
>> +/**
>> * cxlflash_pci_error_detected() - called when a PCI error is detected
>> * @pdev:PCI device struct.
>> * @state: PCI channel state.
>> @@ -2383,16 +2397,14 @@ static pci_ers_result_t
>> cxlflash_pci_error_detected(struct pci_dev *pdev,
>> switch (state) {
>> case pci_channel_io_frozen:
>> cfg->state = STATE_LIMBO;
>> -
>> -/* Turn off legacy I/O */
>> scsi_block_requests(cfg->host);
>> +drain_ioctls(cfg);
>
> So, what kicks any outstanding ioctls back? Let's assume you are in the
> middle of disk_attach
> and you've sent the READ_CAP16 to the device. It appears as if what would
> happen here is we'd
> sit here in cxlflash_pci_error_detected. Eventually, the READ_CAP16 would
> timeout. This would
> wake the SCSI error handler, and end up calling your eh_device_reset handler,
> which would see that
> we are in STATE_LIMBO, where it would then do a wait_event, waiting for us to
> get out of STATE_LIMBO,
> and we would end up in a deadlock.
>
> Rather than implementing a rw semaphore, would it be better to simply make
> the ioctls check the
> state we are in and either wait to get out of EEH state or fail themselves?
We do have the ioctls check the state and wait for EEH to complete
or fail completely in the event that the device is terminating (see
ioctl_common()). The drain exists to create a wait point for ioctls that
have already passed the state check and are active. The CXL services
cannot be called during the recovery window (maybe this requirement
will go away in a future release?), thus the reason for this 'drain'.
To handle it I considered 3 options:
- add state check wraps to all CXL service calls
- create a "running ioctls" count that could be evaluated
- wrap the ioctl in read semaphore and obtain write access to 'wait'
I started with the first option and it quickly made the code very nasty. I then
began implementing the second option and as I was writing the code to wrap
the ioctl with increment/decrement statements, the third option entered my
mind and seemed like a much cleaner solution. Therefore I went with that
approach and did not look back.
With regard to your example, you bring up a good point and we'll need to
do something about that. One thought that comes to mind would be for us
to drop the semaphore before making this type of call (I believe there are
only 2 places like this), reacquiring it when we return, and then checking
the state to make sure we're not in a reset situation.
>
>> rc = cxlflash_mark_contexts_error(cfg);
>> if (unlikely(rc))
>> dev_err(dev, "%s: Failed to mark user contexts!(%d)\n",
>> __func__, rc);
>> term_mc(cfg, UNDO_START);
>> stop_afu(cfg);
>> -
>> return PCI_ERS_RESULT_NEED_RESET;
>> case pci_channel_io_perm_failure:
>> cfg->state = STATE_FAILTERM;
>> diff --git a/drivers/scsi/cxlflash/superpipe.c
>> b/drivers/scsi/cxlflash/superpipe.c
>> index cf2a85d..8a18230 100644
>> --- a/drivers/scsi/cxlflash/superpipe.c
>> +++ b/drivers/scsi/cxlflash/superpipe.c
>> @@ -1214,6 +1214,48 @@ static const struct file_operations null_fops = {
>> };
>>
>> /**
>> + * check_state() - checks and responds to the current adapter state
>> + * @cfg:Internal structure associated with the host.
>> + * @ioctl: Indicates if on an ioctl thread.
>> + *
>> + * This routine can block and should only be used on process context.
>> + * When blocking on an ioctl thread, the ioctl read semaphore should be
>> + * let up to allow for draining actively running ioctls. Also note that
>> + * when waking up from waiting in reset, the state is unknown and must
>> + * be checked again before proceeding.
>> + *
>> + * Return: 0 on success, -errno on failure
>> + */
>> +static int check_state(struct cxlflash_cfg *cfg, bool ioctl)
>
> All your callers appear to set the second parameter to true, so why bother
> having it?
That's a good point. I originally had a case where there was a need for this
but have since removed it. I can fix that in v3.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 12/30] cxlflash: Refine host/device attributes
> On Sep 18, 2015, at 4:34 PM, Brian King wrote: > On 09/16/2015 04:29 PM, Matthew R. Ochs wrote: >> >> +ssize_t bytes = 0; >> +__be64 __iomem *fc_port; >> + >> +if (port >= NUM_FC_PORTS) >> +return 0; >> + >> +fc_port = &afu->afu_map->global.fc_port[port][0]; >> + >> +for (i = 0; i < CXLFLASH_NUM_VLUNS; i++, buf += 22) > > Rather than this bug prone hard coded 22, how about never incrementing buf > and do something > similar to this: > >> +bytes += scnprintf(buf, PAGE_SIZE, "%03d: %016llX\n", >> + i, readq_be(&fc_port[i])); > > bytes += scnprintf(&buf[bytes], PAGE_SIZE, "%03d: %016llX\n", > i, readq_be(&fc_port[i])); > >> +return bytes; >> +} >> + Great suggestion! Will fix in v3. -matt -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] scsi: Fix a bdi reregistration race
Unregister and reregister BDI devices in the proper order. This patch avoids that the following kernel warning can get triggered: WARNING: CPU: 7 PID: 203 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x68/0x80() sysfs: cannot create duplicate filename '/devices/virtual/bdi/8:32' Workqueue: events_unbound async_run_entry_fn Call Trace: [] dump_stack+0x4c/0x65 [] warn_slowpath_common+0x8a/0xc0 [] warn_slowpath_fmt+0x46/0x50 [] sysfs_warn_dup+0x68/0x80 [] sysfs_create_dir_ns+0x7e/0x90 [] kobject_add_internal+0xa8/0x320 [] kobject_add+0x60/0xb0 [] device_add+0x107/0x5e0 [] device_create_groups_vargs+0xd8/0x100 [] device_create_vargs+0x1c/0x20 [] bdi_register+0x63/0x2a0 [] bdi_register_dev+0x27/0x30 [] add_disk+0x1a9/0x4e0 [] sd_probe_async+0x119/0x1d0 [sd_mod] [] async_run_entry_fn+0x4a/0x140 [] process_one_work+0x1d8/0x7c0 [] worker_thread+0x114/0x460 [] kthread+0xf8/0x110 [] ret_from_fork+0x3f/0x70 See also patch "block: destroy bdi before blockdev is unregistered" (commit ID 6cd18e711dd8). Signed-off-by: Bart Van Assche Cc: Hannes Reinecke Cc: Christoph Hellwig Cc: Martin K. Petersen Cc: --- drivers/scsi/scsi_sysfs.c | 14 +++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index b89..5e085d4 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -1084,9 +1084,7 @@ void __scsi_remove_device(struct scsi_device *sdev) device_unregister(&sdev->sdev_dev); transport_remove_device(dev); scsi_dh_remove_device(sdev); - device_del(dev); - } else - put_device(&sdev->sdev_dev); + } /* * Stop accepting new requests and wait until all queuecommand() and @@ -1097,6 +1095,16 @@ void __scsi_remove_device(struct scsi_device *sdev) blk_cleanup_queue(sdev->request_queue); cancel_work_sync(&sdev->requeue_work); + /* +* Remove the device after blk_cleanup_queue() has been called such +* a possible bdi_register() call with the same name occurs after +* blk_cleanup_queue() has called bdi_destroy(). +*/ + if (sdev->is_visible) + device_del(dev); + else + put_device(&sdev->sdev_dev); + if (sdev->host->hostt->slave_destroy) sdev->host->hostt->slave_destroy(sdev); transport_destroy_device(dev); -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
