Re: [PATCH v4 69/78] ncr5380: Fix whitespace in comments using regexp
On Sat, 2 Jan 2016, Joe Perches wrote:
> On Sun, 2016-01-03 at 16:06 +1100, Finn Thain wrote:
> > Hanging indentation was a poor choice for the text inside comments. It
> > has been used in the wrong places and done badly elsewhere. There is
> > little consistency within any file. One fork of the core driver uses
> > tabs for this indentation while the other uses spaces. Better to use
> > flush-left alignment throughout.
> >
> > This patch is the result of the following substitution. It replaces tabs
> > and spaces at the start of a comment line with a single space.
> >
> > perl -i -pe 's,^(\t*[/ ]\*)[ \t]+,$1 ,' drivers/scsi/{atari_,}NCR5380.c
> >
> > This removes some unimportant discrepancies between the two core driver
> > forks so that the important ones become obvious, to facilitate
> > reunification.
>
> I still think this patch is poor at best and
> overall not useful.
Opinions will differ.
> @@ -32,15 +32,15 @@
> > /*
> > * Further development / testing that should be done :
> > * 1. Cleanup the NCR5380_transfer_dma function and DMA operation complete
> > - * code so that everything does the same thing that's done at the
> > - * end of a pseudo-DMA read operation.
> > + * code so that everything does the same thing that's done at the
> > + * end of a pseudo-DMA read operation.
> > *
> > * 2. Fix REAL_DMA (interrupt driven, polled works fine) -
> > - * basically, transfer size needs to be reduced by one
> > - * and the last byte read as is done with PSEUDO_DMA.
> > + * basically, transfer size needs to be reduced by one
> > + * and the last byte read as is done with PSEUDO_DMA.
> > *
> > * 4. Test SCSI-II tagged queueing (I have no devices which support
> > - * tagged queueing)
> > + * tagged queueing)
>
> Numbering 1, 2, then 4 could be improved.
That would be churn. I have other plans for the To Do list. The numbering
will get fixed as items are removed.
--
Re: [PATCH 11/20] scsi_dh_alua: Use workqueue for RTPG
On Thu, Dec 31, 2015 at 02:01:43PM +0100, Hannes Reinecke wrote:
>>> if (tmp_pg) {
>>> spin_unlock(&port_group_lock);
>>> - kfree(pg);
>>> - return tmp_pg;
>>> + kref_put(&pg->kref, release_port_group);
>>> + pg = tmp_pg;
>>> + tmp_pg = NULL;
>>
>> The only thing release_port_group does in addition to the kfree
>> is a list_del on pg->entry. But given that we never added
>> the pg to a list it doesn't need to be deleted, and it can't
>> have another reference. Why this change?
>>
> Mainly for symmetry; with this patch we're always calling kref_put() to
> delete the port_group structure.
Given that it has never been added it's not actuallt symmetric.
Either way it shouldn't be added in this patch - either use kref_put
from the patch introducing the kref or not use it at all. I would
prefer the second option.
>>
>> pg_found should be pg_updated, right?
>>
>>> + if (pg_found)
>>> + synchronize_rcu();
>>> + if (old_pg) {
>>> + if (old_pg->rtpg_sdev)
>>> + flush_delayed_work(&old_pg->rtpg_work);
>>> + kref_put(&old_pg->kref, release_port_group);
>>> + }
>>
>> This code looks odd. I can't see why we need a synchronize_rcu here.
>> The only thing we should need is a kfree_rcu for the final free in
>> release_port_group. I also don't quite understand the flush_delayed_work.
>> As far as I can tell we only need it if rtpg_sdev is the sdev passed
>> in, so it we probably should check for that.
>>
> rtpg_sdev is set whenever the port_group structure needs or is scheduled
> for alua_rtpg_work(), ie whenever some action needs to be taken.
> As the sdev might be a different sdev than that one we've been called from
> (a port_group might have several sdevs pointing to it), the existence of an
> sdev signals that we need to flush the workqueue item.
So why do we only need to flush it for some kref_put callers and not the
others?
--
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 12/20] scsi_dh_alua: Allow workqueue to run synchronously
On Thu, Dec 31, 2015 at 02:54:18PM +0100, Hannes Reinecke wrote: > ATM the only ones I know of is NetApp (both FAS and E-series; E-series > requires it, and FAS benefits greatly). > But this is not to say that these are the only ones, _and_ the more obvious > approach would be to add the handling to multipath-tools settings, so I > really would like to keep the module option for now. Anything involving multipath-tools settings is a giant nightmare. We need to get it rifght in the kernel so that multipathing can work out of the box. And the right place for SCSI-level quirks are the devlist entries. Module options for hardware specific behavior are a horrible idea - you can be connect to different devies, which is a very common setup. -- 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: NMI watchdog: BUG: soft lockup - CPU#1 stuck for 22s! [kworker/1:1:29]
On Thu, Dec 31, 2015 at 06:04:15PM +0100, Sebastian Herbszt wrote: > I still get this on 4.4.0-rc7-1.g276c9f4-default. Since this did not > happen on 4.3 I checked the scsi changes and found the following commit: > > scsi: restart list search after unlock in scsi_remove_target > > Christoph, can it cause this issue? Apparently yes. Bad hard a patch to avoid this when he resubmitted my patch, which for some reason didn't get apply. Can you grab it from the list archives and give it a try? -- 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 v4 69/78] ncr5380: Fix whitespace in comments using regexp
On Sat, 02 Jan 2016 23:54:28 -0800
Joe Perches wrote:
> On Sun, 2016-01-03 at 16:06 +1100, Finn Thain wrote:
> > Hanging indentation was a poor choice for the text inside comments. It
> > has been used in the wrong places and done badly elsewhere. There is
> > little consistency within any file. One fork of the core driver uses
> > tabs for this indentation while the other uses spaces. Better to use
> > flush-left alignment throughout.
> >
> > This patch is the result of the following substitution. It replaces tabs
> > and spaces at the start of a comment line with a single space.
> >
> > perl -i -pe 's,^(\t*[/ ]\*)[ \t]+,$1 ,' drivers/scsi/{atari_,}NCR5380.c
> >
> > This removes some unimportant discrepancies between the two core driver
> > forks so that the important ones become obvious, to facilitate
> > reunification.
>
> I still think this patch is poor at best and
> overall not useful.
I would beg to differ. As a tool for understanding the differences as you
step through the versions it's invaluable.
Alan
--
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: NMI watchdog: BUG: soft lockup - CPU#1 stuck for 22s! [kworker/1:1:29]
Christoph Hellwig wrote: > On Thu, Dec 31, 2015 at 06:04:15PM +0100, Sebastian Herbszt wrote: > > I still get this on 4.4.0-rc7-1.g276c9f4-default. Since this did not > > happen on 4.3 I checked the scsi changes and found the following commit: > > > > scsi: restart list search after unlock in scsi_remove_target > > > > Christoph, can it cause this issue? > > Apparently yes. Bad hard a patch to avoid this when he resubmitted > my patch, which for some reason didn't get apply. Can you grab it > from the list archives and give it a try? Do you mean the following patch? "[PATCH v2] Separate target visibility from reaped state information" [1] I can give it a try but it will likely take a few days. [1] http://marc.info/?l=linux-scsi&m=144771953020415&w=2 Sebastian -- 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 v4 69/78] ncr5380: Fix whitespace in comments using regexp
On Sun, 2016-01-03 at 14:10 +, One Thousand Gnomes wrote:
> On Sat, 02 Jan 2016 23:54:28 -0800
> Joe Perches wrote:
>
> > On Sun, 2016-01-03 at 16:06 +1100, Finn Thain wrote:
> > > Hanging indentation was a poor choice for the text inside comments. It
> > > has been used in the wrong places and done badly elsewhere. There is
> > > little consistency within any file. One fork of the core driver uses
> > > tabs for this indentation while the other uses spaces. Better to use
> > > flush-left alignment throughout.
> > >
> > > This patch is the result of the following substitution. It replaces tabs
> > > and spaces at the start of a comment line with a single space.
> > >
> > > perl -i -pe 's,^(\t*[/ ]\*)[ \t]+,$1 ,' drivers/scsi/{atari_,}NCR5380.c
> > >
> > > This removes some unimportant discrepancies between the two core driver
> > > forks so that the important ones become obvious, to facilitate
> > > reunification.
> >
> > I still think this patch is poor at best and
> > overall not useful.
>
> I would beg to differ. As a tool for understanding the differences as you
> step through the versions it's invaluable.
This removes intentional formatting that
makes reading comments easier.
diff -w works well.
--
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 v4 69/78] ncr5380: Fix whitespace in comments using regexp
> > I would beg to differ. As a tool for understanding the differences as you > > step through the versions it's invaluable. > > This removes intentional formatting that > makes reading comments easier. And this matters at the end of the patch series because ? -- 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 v4 69/78] ncr5380: Fix whitespace in comments using regexp
(using an email address for James that shouldn't bounce) On Sun, 2016-01-03 at 21:29 +, One Thousand Gnomes wrote: > > > I would beg to differ. As a tool for understanding the > > > differences as you > > > step through the versions it's invaluable. > > > > This removes intentional formatting that > > makes reading comments easier. > > And this matters at the end of the patch series because ? It removes intentional formatting that makes reading comments easier. And diff -w still works just fine. -- 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 v4 69/78] ncr5380: Fix whitespace in comments using regexp
On Sun, 2016-01-03 at 22:05 +, One Thousand Gnomes wrote: > On Sun, 03 Jan 2016 13:46:22 -0800 > Joe Perches wrote: > > > (using an email address for James that shouldn't bounce) > > > > On Sun, 2016-01-03 at 21:29 +, One Thousand Gnomes wrote: > > > > > I would beg to differ. As a tool for understanding the > > > > > differences as you > > > > > step through the versions it's invaluable. > > > > > > > > This removes intentional formatting that > > > > makes reading comments easier. > > > > > > And this matters at the end of the patch series because ? > > > > It removes intentional formatting that makes > > reading comments easier. > > I'm missing something here - the final result of the merge is completely > and easily readable. Likewise I can work through it and review the code > and compare them for the platform I care about (Mac68K) > > > And diff -w still works just fine. > > So you think 70+ patches should be redone because of that, when the end > result is a clean driver anyway ??? Nope, I think patch 69 should simply not be applied. I don't know and don't care if patches 70 through 78 depend on any changes made by patch 69. I'd expect no dependencies exist though. It doesn't matter much in any case. cheers, Joe -- 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 RESEND] megaraid:Fix for loop termination statment in the function process_fw_state_change_wq
> -Original Message-
> From: Nicholas Krause [mailto:xerofo...@gmail.com]
> Sent: Friday, January 01, 2016 12:06 PM
> To: kashyap.de...@avagotech.com
> Cc: sumit.sax...@avagotech.com; uday.ling...@avagotech.com;
> jbottom...@odin.com; martin.peter...@oracle.com;
> megaraidlinux@avagotech.com; linux-scsi@vger.kernel.org; linux-
> ker...@vger.kernel.org
> Subject: [PATCH RESEND] megaraid:Fix for loop termination statment in
the
> function process_fw_state_change_wq
>
> This fixes the for loop terimation for the waiting period between the
first and
> second init to make the variable wait terminate at the value of 20
rather then 30
> in other to follow the comments about this for loop of waiting for about
20
> seconds rather then
> 30 in the function fw_state_change_wq between initializations.
>
> Signed-off-by: Nicholas Krause
> ---
> drivers/scsi/megaraid/megaraid_sas_base.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
> b/drivers/scsi/megaraid/megaraid_sas_base.c
> index 71b884d..22fd333 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> @@ -3168,9 +3168,8 @@ process_fw_state_change_wq(struct work_struct
> *work)
> "state 2 starting...\n");
>
> /*waitting for about 20 second before start the second
init*/
> - for (wait = 0; wait < 30; wait++) {
> + for (wait = 0; wait < 20; wait++)
> msleep(1000);
> - }
>
> if (megasas_transition_to_ready(instance, 1)) {
> printk(KERN_NOTICE "megaraid_sas:adapter not
> ready\n");
There should be 30 seconds delay here, comments needs to be rectified
here. I will take care of this in next patch set.
> --
> 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
Re: [PATCH 12/20] target/tmr: LUN reset cause cmd premature free.
On 12/08/2015 03:48 AM, Christoph Hellwig wrote: On Mon, Dec 07, 2015 at 07:48:59PM -0500, Himanshu Madhani wrote: From: Quinn Tran During LUN/Target reset, the TMR code attempt to intercept cmds and try to aborted them. Current code assume cmds are always intercepted at the back end device. The cleanup code would issue a "queue_status() & check_stop_free()" to terminate the command. However, when a cmd is intercepted at the front end/Fabric layer, current code introduce premature free or cause Fabric to double free. When command is intercepted at Fabric layer, it means a check_stop_free(cmd_kref--) has been called. The extra check_stop_free in the Lun Reset cleanup code causes early free. When a cmd in the Fabric layer is completed, the normal free code adds another another free which introduce a double free. To fix the issue: - add a new flag/CMD_T_SENT_STATUS to track command that have made it down to fabric layer after back end good/bad completion. - if cmd reach Fabric Layer at Lun Reset time, add an extra cmd_kref count to prevent premature free. This seems lke something solved by Bart's abort rewrite in a much nicer way. All this special casing based on magic flags isn't maintainable in the long run. Hello Himanshu and Christoph, I am currently working on addressing the review comments on my target core patch series and will repost that patch series this week. Bart. -- 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
