Re: [PATCH v2 2/2] libata-core: do not set dev->max_sectors for LBA48 devices

[One Thousand Gnomes]

> Tom> is totally irrational for a general default. I mean, given that it
> Tom> was 1024 (512k), try to double it? Fine. Try to quadruple it?
> Tom> Alright.  We'll need to deal with some alignment / boundary issue
> Tom> (like the typical 65535 vs 65536 case)? Okay let's do it. But
> Tom> what's the sense in picking a random RAID configuartion as the base
> Tom> to decide the default?  
> 
> I agree that the new default is completely arbitrary. And I think there
> was a bit of controversy at the time. However, the numbers were
> compelling so the change went in.

For older SCSI and especially ATA drives (and it wouldn't surprise me if
it is true of modern ones) there are also huge latency tradeoffs. Before
you jump up and down about numbers what are the latency numbers like on
classic ATA drives with that sized block I/O. You could easily up your
RAID numbers while wrecking realtime and desktop performance.

Without NCQ you certainly don't want huge I/O's blocking up the
controller, with NCQ it depends if the firmware these days has improved
and understands any kind of fairness or like the early ones and older
SCSI drives will happily starve requests.

Conceptually it also seems wrong - if you want to avoid partial stripe
writes on a consumer grade disk subsystem then use smaller stripes.

At the ATA level we can detect both the presence of command queueing
ability, and also whether the device is spinnning rust or not, so it may
be smarter defaults could be done based upon whether the device is an SSD
or not.

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: [PATCH v2] [SCSI] dpt_i2o: use proper pci driver

[One Thousand Gnomes]

On Wed, 17 Feb 2016 17:50:14 +0530
Sudip Mukherjee  wrote:

> This is a pci device but was not done in the usual way a pci driver is
> done. Convert the driver into a proper pci driver.

This looks completely wrong. Please read the I2O 1.5 specification
document before playing with that code. You can't simply convert it to a
standard PCI driver as all the IOPs are supposed to be detected and then
the correct initialization sequence executed across the set of IOPs. IOPs
are allowed to talk to one another and the system table binds it all
together.

If you do hot plug then you need to follow the specification and do
all the extra notifications. Unless you've got multiple FC909/FC920
fibechannel cards or similar to test with I would just leave this well
alone. Your original simple patch is *MUCH* safer in this specific case.

So NAK

Alan


> 
> Signed-off-by: Sudip Mukherjee 
> ---
> 
> v1: only build warning related to "dptids defined but not used" was
> fixed using #ifdef
> 
>  drivers/scsi/dpt_i2o.c | 269 
> ++---
>  drivers/scsi/dpti.h|   5 +-
>  2 files changed, 120 insertions(+), 154 deletions(-)
> 
> diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
> index d4cda5e..9f50d1ae 100644
> --- a/drivers/scsi/dpt_i2o.c
> +++ b/drivers/scsi/dpt_i2o.c
> @@ -111,6 +111,7 @@ static int sys_tbl_len;
>  
>  static adpt_hba* hba_chain = NULL;
>  static int hba_count = 0;
> +static bool control_reg;
>  
>  static struct class *adpt_sysfs_class;
>  
> @@ -187,118 +188,6 @@ static struct pci_device_id dptids[] = {
>  };
>  MODULE_DEVICE_TABLE(pci,dptids);
>  
> -static int adpt_detect(struct scsi_host_template* sht)
> -{
> - struct pci_dev *pDev = NULL;
> - adpt_hba *pHba;
> - adpt_hba *next;
> -
> - PINFO("Detecting Adaptec I2O RAID controllers...\n");
> -
> -/* search for all Adatpec I2O RAID cards */
> - while ((pDev = pci_get_device( PCI_DPT_VENDOR_ID, PCI_ANY_ID, pDev))) {
> - if(pDev->device == PCI_DPT_DEVICE_ID ||
> -pDev->device == PCI_DPT_RAPTOR_DEVICE_ID){
> - if(adpt_install_hba(sht, pDev) ){
> - PERROR("Could not Init an I2O RAID device\n");
> - PERROR("Will not try to detect others.\n");
> - return hba_count-1;
> - }
> - pci_dev_get(pDev);
> - }
> - }
> -
> - /* In INIT state, Activate IOPs */
> - for (pHba = hba_chain; pHba; pHba = next) {
> - next = pHba->next;
> - // Activate does get status , init outbound, and get hrt
> - if (adpt_i2o_activate_hba(pHba) < 0) {
> - adpt_i2o_delete_hba(pHba);
> - }
> - }
> -
> -
> - /* Active IOPs in HOLD state */
> -
> -rebuild_sys_tab:
> - if (hba_chain == NULL) 
> - return 0;
> -
> - /*
> -  * If build_sys_table fails, we kill everything and bail
> -  * as we can't init the IOPs w/o a system table
> -  */ 
> - if (adpt_i2o_build_sys_table() < 0) {
> - adpt_i2o_sys_shutdown();
> - return 0;
> - }
> -
> - PDEBUG("HBA's in HOLD state\n");
> -
> - /* If IOP don't get online, we need to rebuild the System table */
> - for (pHba = hba_chain; pHba; pHba = pHba->next) {
> - if (adpt_i2o_online_hba(pHba) < 0) {
> - adpt_i2o_delete_hba(pHba);  
> - goto rebuild_sys_tab;
> - }
> - }
> -
> - /* Active IOPs now in OPERATIONAL state */
> - PDEBUG("HBA's in OPERATIONAL state\n");
> -
> - printk("dpti: If you have a lot of devices this could take a few 
> minutes.\n");
> - for (pHba = hba_chain; pHba; pHba = next) {
> - next = pHba->next;
> - printk(KERN_INFO"%s: Reading the hardware resource table.\n", 
> pHba->name);
> - if (adpt_i2o_lct_get(pHba) < 0){
> - adpt_i2o_delete_hba(pHba);
> - continue;
> - }
> -
> - if (adpt_i2o_parse_lct(pHba) < 0){
> - adpt_i2o_delete_hba(pHba);
> - continue;
> - }
> - adpt_inquiry(pHba);
> - }
> -
> - adpt_sysfs_class = class_create(THIS_MODULE, "dpt_i2o");
> - if (IS_ERR(adpt_sysfs_class)) {
> - printk(KERN_WARNING"dpti: unable to create dpt_i2o class\n");
> - adpt_sysfs_class = NULL;
> - }
> -
> - for (pHba = hba_chain; pHba; pHba = next) {
> - next = pHba->next;
> - if (adpt_scsi_host_alloc(pHba, sht) < 0){
> - adpt_i2o_delete_hba(pHba);
> - continue;
> - }
> - pHba->initialized = TRUE;
> - pHba->state &= ~DPTI_STATE_RESET;
> - if (adpt_sysfs_class) {
> -  

Re: [PATCH v4 69/78] ncr5380: Fix whitespace in comments using regexp

[One Thousand Gnomes]

> > 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

[One Thousand Gnomes]

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: [PATCH v3 66/77] ncr5380: Fix soft lockups

[One Thousand Gnomes]

On Tue, 22 Dec 2015 12:18:44 +1100
Finn Thain  wrote:

> Because of the rudimentary design of the chip, it is necessary to poll the
> SCSI bus signals during PIO and this tends to hog the CPU. The driver will
> accept new commands while others execute, and this causes a soft lockup
> because the workqueue item will not terminate until the issue queue is
> emptied.
> 
> When exercising dmx3191d using sequential IO from dd, the driver is sent
> 512 KiB WRITE commands and 128 KiB READs. For a PIO transfer, the rate is
> is only about 300 KiB/s, so these are long-running commands. And although
> PDMA may run at several MiB/s, interrupts are disabled for the duration
> of the transfer.
> 
> Fix the unresponsiveness and soft lockup issues by calling cond_resched()
> after each command is completed and by limiting max_sectors for drivers
> that don't implement real DMA.

Is there a reason for not doing some limiting in the DMA case too. A 512K
write command even with DMA on a low end 68K box introduces a second of
latency before another I/O can be scheduled ?

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: [patch RESEND] atp870u: 64 bit bug in atp885_init()

[One Thousand Gnomes]

On Wed, 9 Dec 2015 13:24:53 +0300
Dan Carpenter  wrote:

> On 64 bit CPUs there is a memory corruption bug on probe().  It should
> be a u32 pointer instead of an unsigned long pointer or we write past
> the end of the setupdata[] array.
> 
> Signed-off-by: Dan Carpenter 
> Reviewed-by: Hannes Reinecke 
> ---
> Resending because we have shuffled the code around so the patch needed
> to be refreshed against linux-next.  Although I do wonder why we are
> still working on this code since it has never worked on 64 bit systems
> so probably all the users gave up a decade ago.

So this is untested ? If so please make it very clear in the commit
message because the kernel is IMHO getting too full of polished, neat,
recently modified, never tested, never used code.

I agree it would be better if the driver was simply deleted. I've not
even seen an ATP870 bug report in years.

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: [patch RESEND] atp870u: 64 bit bug in atp885_init()

[One Thousand Gnomes]

On Wed, 9 Dec 2015 16:45:12 +0300
Dan Carpenter  wrote:

> Everyone knows I didn't test it but it's an obvious one line fix for
> memory corruption.  If no one uses the code, at least this is harmless
> and silences a static checker warning.
> 
> In olden times we used to say, "Oh this bounds checking is crap but it's
> root only so let's leave it alone."  But these days we just fix it.
> It's easier to just fix everything instead of trying to decide which
> bugs are critical.

Unfortunately it's all too easy to look down 50 commit messages to an
apaprently active file all "fixing small bugs" or "correcting indenting"
without realising that every single one of them should have been tagged

"[UNTESTED]: "

so that anyone looking at the code can see immediately its historical
hazardous waste.

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: [patch RESEND] atp870u: 64 bit bug in atp885_init()

[One Thousand Gnomes]

> You should just delete that code along with the Kconfig and Makefile
> entries.  Don't bother moving it to staging.  The move to staging is
> supposed to give people one last chance to yell if they absolutely need
> the code.  No one has compiled this code for years so no one will miss
> it.

And for stuff which might be worth saving (eg something that looks rather
broken but has possibly got users) the driver goes into staging
in its own directory and the Makefile and Kconfig entry for it move into
the staging directory with the hope that someone screams and maintains it.

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: [PATCH 3/3] libata: support host-aware ZAC devices

[One Thousand Gnomes]

On Tue, 31 Mar 2015 10:52:43 +0200
Hannes Reinecke h...@suse.de wrote:

 Byte 69 bits 0:1 in the IDENTIFY DEVICE data are proposed
 to indicate a host-aware ZAC device.
 And whenever we detect a ZAC-compatible device we should
 be displaying the zoned block characteristics VPD page.

I don't think this belongs upstream as is - because this is a proposed
- it might change and then we have Linux boxes in the field randomly
misinterpreting bits if instead other bits are used or the definition
changes.

Until it's officially in the spec there ought IMHO to be a boot option to
enable the test.

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: [PATCH 1/3] libata: Whitelist SSDs that are known to properly return zeroes after TRIM

[One Thousand Gnomes]

On Mon, 8 Dec 2014 10:15:59 -0500
Theodore Ts'o ty...@mit.edu wrote:

 On Fri, Dec 05, 2014 at 10:58:09PM +, Elliott, Robert (Server Storage) 
 wrote:
   I have a Crucial_CT256MX1 (i.e. MX100) and it does reliably zero.
  
  make me concerned about this whitelist approach.
  
  I think you need a manufacturer assertion that this is indeed
  the design intent; you cannot be certain based on observation
  from outside.
 
 How is this different from a manufacturer assertion that they follow a
 SCSI or ATA standard?  There have been cases in the distant past
 (fortunately) of disk manufacturers that ignored a CACHE FLUSH command
 just to get higher Winbench scores.  Does that mean we can't trust
 them to do anything right?

At the time they never promised to honour cache flush. The reason it was
became mandatory in the specification was in part so that the vendors
could all force each other to play fair. If its optional then it's
tough..., if they say they meet the standard it's class action 8)

If this is a promise then it ought to be good 

James:  USB devices tend to supply their own driver

has not been true for some years now. Microsoft provide an in-box driver
and vendors have the choice of using that or certifying their own via
WHQL, which is a bit like choosing between free ice cream and banging
your head against a plank cover in nails.

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: [regression] fix 32-bit breakage in block device read(2) (was Re: 32-bit bug in iovec iterator changes)

[One Thousand Gnomes]

On Mon, 23 Jun 2014 11:43:02 -0400
Theodore Ts'o ty...@mit.edu wrote:

 On Mon, Jun 23, 2014 at 08:44:40AM +0100, Al Viro wrote:
  
  OK, here it is, hopefully with sufficient comments:
 
 The comments look really good.  I assume you'll get this to
 Linus in time for 3.16-rc3?

Fixes the 32GB 'can't partition' bug I reported too.

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: [RFC 1/1] drivers/scsi/bfa/bfad_debugfs.c: replace 2 kzalloc/copy_from_user to memdup_user

[One Thousand Gnomes]

On Fri, 30 May 2014 19:07:36 +0200
Fabian Frederick f...@skynet.be wrote:

 (memdup_user can be used to replace kmalloc/copy_from_user. Not sure if it's 
 ok with kzalloc ...)


 + kern_buf = memdup_user((void __user *)buf, nbytes);

 + if (IS_ERR(kern_buf)) {
 + printk(KERN_INFO bfad[%d]: Failed to copy buffer\n,
 +bfad-inst_no);

While you are tidying up lose the printk KERN_INFO stuff here, it really
isn't needed.

Looks good to me and it also fixes the incorrect return of -ENOMEM the
old code had on a copy_from_user failing.

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: [PATCH/RESEND v2 1/2] Hard disk resume time optimization, asynchronous ata_port_resume

[One Thousand Gnomes]

 thus allowing the UI to come online sooner. There may be a short period after 
 resume where the disks are still spinning up in the background, but the user 
 shouldn't notice since the OS can function with the data left in RAM.

I wonder how many marginal power supplies this will find 8)

I still think it's the right thing to do. In the SCSI world a bit more
caution was needed however.

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