NetBSD Problem Report #39514

From www@NetBSD.org  Wed Sep 10 20:00:42 2008
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by narn.NetBSD.org (Postfix) with ESMTP id 09ECB63BC30
	for <gnats-bugs@gnats.netbsd.org>; Wed, 10 Sep 2008 20:00:42 +0000 (UTC)
Message-Id: <20080910200041.CCE9563B93C@narn.NetBSD.org>
Date: Wed, 10 Sep 2008 20:00:41 +0000 (UTC)
From: xtraeme@gmail.com
Reply-To: xtraeme@gmail.com
To: gnats-bugs@NetBSD.org
Subject: ataraid(4): basic bio(4) support
X-Send-Pr-Version: www-1.0

>Number:         39514
>Category:       kern
>Synopsis:       ataraid(4): basic bio(4) support
>Confidential:   no
>Severity:       non-critical
>Priority:       high
>Responsible:    tron
>State:          closed
>Class:          change-request
>Submitter-Id:   net
>Arrival-Date:   Wed Sep 10 20:05:00 +0000 2008
>Closed-Date:    Mon Sep 15 11:56:09 +0000 2008
>Last-Modified:  Mon Sep 15 11:56:09 +0000 2008
>Originator:     Juan RP
>Release:        Latest
>Organization:
>Environment:
NetBSD sasha 4.99.72 NetBSD 4.99.72 (MASTER.DEBUG) #15: Wed Sep 10 21:47:55 CEST 2008  juan@sasha:/home/juan/build/amd64/obj/sys/arch/amd64/compile/MASTER.DEBUG amd64
>Description:
The ataraid(4) driver doesn't have support to query the status of
the logical disks. There are some printfs if something bad happens,
but that's not enough.

I've added basic support for ataraid(4) to show status about its
logical disk and physical disks associated. Here's an example
of the output with a RAID 1 volume on an Intel MatrixRAID
controller.

$ sudo bioctl ld0 show
Volume Status       Size         Device/Label    Level Stripe
=============================================================
     0 Online       234G          ld0 Volume0   RAID 1    N/A 
   0:0 Online       234G            0:3.0 wd3 <WDC WD2500YS-01SHB1 20.06C06>
   0:1 Online       234G            0:4.0 wd4 <WDC WD2500YS-01SHB1 20.06C06>
$

Will send a patch once PR kern/39511 is applied (I think tron@ will
handle it).
>How-To-Repeat:

>Fix:

>Release-Note:

>Audit-Trail:
From: Juan RP <xtraeme@gmail.com>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/39514: ataraid(4): basic bio(4) support
Date: Thu, 11 Sep 2008 22:58:41 +0200

 Here's the patch against latest HEAD:

 Index: sys/dev/ata/ata_raidvar.h
 ===================================================================
 RCS file: /cvsroot/src/sys/dev/ata/ata_raidvar.h,v
 retrieving revision 1.8
 diff -b -u -p -r1.8 ata_raidvar.h
 --- sys/dev/ata/ata_raidvar.h	11 Sep 2008 11:08:50 -0000	1.8
 +++ sys/dev/ata/ata_raidvar.h	11 Sep 2008 20:57:03 -0000
 @@ -98,6 +98,8 @@ struct ataraid_array_info {
  	u_int	aai_offset;		/* component start offset */
  	u_int	aai_reserved;		/* component reserved sectors */

 +	char	aai_name[32];		/* array volume name */
 +
  	struct ataraid_disk_info aai_disks[ATA_RAID_MAX_DISKS];
  };

 Index: sys/dev/ata/ld_ataraid.c
 ===================================================================
 RCS file: /cvsroot/src/sys/dev/ata/ld_ataraid.c,v
 retrieving revision 1.29
 diff -b -u -p -r1.29 ld_ataraid.c
 --- sys/dev/ata/ld_ataraid.c	9 Sep 2008 12:45:39 -0000	1.29
 +++ sys/dev/ata/ld_ataraid.c	11 Sep 2008 20:57:04 -0000
 @@ -42,11 +42,14 @@
   * controllers we're dealing with (Promise, etc.) only support
   * configuration data on the component disks, with the BIOS supporting
   * booting from the RAID volumes.
 + *
 + * bio(4) support was written by Juan Romero Pardines <xtraeme@gmail.com>.
   */

  #include <sys/cdefs.h>
  __KERNEL_RCSID(0, "$NetBSD: ld_ataraid.c,v 1.29 2008/09/09 12:45:39 tron Exp $");

 +#include "bio.h"
  #include "rnd.h"

  #include <sys/param.h>
 @@ -66,6 +69,13 @@ __KERNEL_RCSID(0, "$NetBSD: ld_ataraid.c
  #if NRND > 0
  #include <sys/rnd.h>
  #endif
 +#if NBIO > 0
 +#include <dev/ata/atavar.h>
 +#include <dev/ata/atareg.h>
 +#include <dev/ata/wdvar.h>
 +#include <dev/biovar.h>
 +#include <dev/scsipi/scsipiconf.h> /* for scsipi_strvis() */
 +#endif

  #include <miscfs/specfs/specdev.h>

 @@ -92,6 +102,14 @@ static int	ld_ataraid_start_span(struct 
  static int	ld_ataraid_start_raid0(struct ld_softc *, struct buf *);
  static void	ld_ataraid_iodone_raid0(struct buf *);

 +#if NBIO > 0
 +static int	ld_ataraid_bioctl(device_t, u_long, void *);
 +static int	ld_ataraid_bioinq(struct ld_ataraid_softc *, struct bioc_inq *);
 +static int	ld_ataraid_biovol(struct ld_ataraid_softc *, struct bioc_vol *);
 +static int	ld_ataraid_biodisk(struct ld_ataraid_softc *,
 +				   struct bioc_disk *);
 +#endif
 +
  CFATTACH_DECL_NEW(ld_ataraid, sizeof(struct ld_ataraid_softc),
      ld_ataraid_match, ld_ataraid_attach, NULL, NULL);

 @@ -233,6 +251,11 @@ ld_ataraid_attach(device_t parent, devic
  	}

   finish:
 +#if NBIO > 0
 +	if (bio_register(self, ld_ataraid_bioctl) != 0)
 +		panic("%s: bioctl registration failed\n",
 +		    device_xname(ld->sc_dv));
 +#endif
  	ldattach(ld);
  }

 @@ -541,3 +564,139 @@ ld_ataraid_dump(struct ld_softc *sc, voi

  	return (EIO);
  }
 +
 +#if NBIO > 0
 +static int
 +ld_ataraid_bioctl(device_t self, u_long cmd, void *addr)
 +{
 +	struct ld_ataraid_softc *sc = device_private(self);
 +	int error = 0;
 +
 +	switch (cmd) {
 +	case BIOCINQ:
 +		error = ld_ataraid_bioinq(sc, (struct bioc_inq *)addr);
 +		break;
 +	case BIOCVOL:
 +		error = ld_ataraid_biovol(sc, (struct bioc_vol *)addr);
 +		break;
 +	case BIOCDISK:
 +		error = ld_ataraid_biodisk(sc, (struct bioc_disk *)addr);
 +		break;
 +	default:
 +		error = ENOTTY;
 +		break;
 +	}
 +
 +	return error;
 +}
 +
 +static int
 +ld_ataraid_bioinq(struct ld_ataraid_softc *sc, struct bioc_inq *bi)
 +{
 +	struct ataraid_array_info *aai = sc->sc_aai;
 +
 +	/* there's always one volume per ld device */
 +	bi->bi_novol = 1;
 +	bi->bi_nodisk = aai->aai_ndisks;
 +
 +	return 0;
 +}
 +
 +static int
 +ld_ataraid_biovol(struct ld_ataraid_softc *sc, struct bioc_vol *bv)
 +{
 +	struct ataraid_array_info *aai = sc->sc_aai;
 +	struct ld_softc *ld = &sc->sc_ld;
 +
 +	/* Fill in data for _this_ volume */
 +	bv->bv_percent = -1;
 +	bv->bv_seconds = 0;
 +
 +	switch (aai->aai_status) {
 +	case AAI_S_READY:
 +		bv->bv_status = BIOC_SVONLINE;
 +		break;
 +	case AAI_S_DEGRADED:
 +		bv->bv_status = BIOC_SVDEGRADED;
 +		break;
 +	}
 +
 +	bv->bv_size = ld->sc_secsize * ld->sc_secperunit;
 +
 +	switch (aai->aai_level) {
 +	case AAI_L_SPAN:
 +	case AAI_L_RAID0:
 +		bv->bv_stripe_size = aai->aai_interleave;
 +		bv->bv_level = 0;
 +		break;
 +	case AAI_L_RAID1:
 +		bv->bv_stripe_size = 0;
 +		bv->bv_level = 1;
 +		break;
 +	case AAI_L_RAID5:
 +		bv->bv_stripe_size = aai->aai_interleave;
 +		bv->bv_level = 5;
 +		break;
 +	}
 +
 +	bv->bv_nodisk = aai->aai_ndisks;
 +	strlcpy(bv->bv_dev, device_xname(ld->sc_dv), sizeof(bv->bv_dev));
 +	if (aai->aai_name)
 +		strlcpy(bv->bv_vendor, aai->aai_name,
 +		    sizeof(bv->bv_vendor));
 +
 +	return 0;
 +}
 +
 +static int
 +ld_ataraid_biodisk(struct ld_ataraid_softc *sc, struct bioc_disk *bd)
 +{
 +	struct ataraid_array_info *aai = sc->sc_aai;
 +	struct ataraid_disk_info *adi;
 +	struct ld_softc *ld = &sc->sc_ld;
 +	struct atabus_softc *atabus;
 +	struct wd_softc *wd;
 +	char model[81], serial[41], rev[17];
 +
 +	/* sanity check */
 +	if (bd->bd_diskid > aai->aai_ndisks)
 +		return EINVAL;
 +
 +	adi = &aai->aai_disks[bd->bd_diskid];
 +	atabus = device_private(device_parent(adi->adi_dev));
 +	wd = device_private(adi->adi_dev);
 +
 +	/* fill in data for _this_ disk */
 +	switch (adi->adi_status) {
 +	case ADI_S_ONLINE | ADI_S_ASSIGNED:
 +		bd->bd_status = BIOC_SDONLINE;
 +		break;
 +	case ADI_S_SPARE:
 +		bd->bd_status = BIOC_SDHOTSPARE;
 +		break;
 +	default:
 +		bd->bd_status = BIOC_SDOFFLINE;
 +		break;
 +	}
 +
 +	bd->bd_channel = 0;
 +	bd->bd_target = atabus->sc_chan->ch_channel;
 +	bd->bd_lun = 0;
 +	bd->bd_size = (wd->sc_capacity * ld->sc_secsize) - aai->aai_reserved;
 +
 +	strlcpy(bd->bd_procdev, device_xname(adi->adi_dev),
 +	    sizeof(bd->bd_procdev));
 +
 +	scsipi_strvis(serial, sizeof(serial), wd->sc_params.atap_serial,
 +	    sizeof(wd->sc_params.atap_serial));
 +	scsipi_strvis(model, sizeof(model), wd->sc_params.atap_model,
 +	    sizeof(wd->sc_params.atap_model));
 +	scsipi_strvis(rev, sizeof(rev), wd->sc_params.atap_revision,
 +	    sizeof(wd->sc_params.atap_revision));
 +
 +	snprintf(bd->bd_vendor, sizeof(bd->bd_vendor), "%s %s", model, rev);
 +	strlcpy(bd->bd_serial, serial, sizeof(bd->bd_serial));
 +
 +	return 0;
 +}
 +#endif /* NBIO > 0 */
 Index: sys/dev/ata/ata_raid_adaptec.c
 ===================================================================
 RCS file: /cvsroot/src/sys/dev/ata/ata_raid_adaptec.c,v
 retrieving revision 1.8
 diff -b -u -p -r1.8 ata_raid_adaptec.c
 --- sys/dev/ata/ata_raid_adaptec.c	18 Mar 2008 20:46:36 -0000	1.8
 +++ sys/dev/ata/ata_raid_adaptec.c	11 Sep 2008 20:57:04 -0000
 @@ -160,6 +160,9 @@ ata_raid_read_config_adaptec(struct wd_s
  		aai->aai_cylinders = aai->aai_capacity / (63 * 255);
  		aai->aai_offset = 0;
  		aai->aai_reserved = 17;
 +		if (info->configs[0].name)
 +			strlcpy(aai->aai_name, info->configs[0].name,
 +			    sizeof(aai->aai_name));

  		/* XXX - bogus.  RAID1 shouldn't really have an interleave */
  		if (aai->aai_interleave == 0)
 Index: sys/dev/ata/ata_raid_intel.c
 ===================================================================
 RCS file: /cvsroot/src/sys/dev/ata/ata_raid_intel.c,v
 retrieving revision 1.1
 diff -b -u -p -r1.1 ata_raid_intel.c
 --- sys/dev/ata/ata_raid_intel.c	11 Sep 2008 11:08:50 -0000	1.1
 +++ sys/dev/ata/ata_raid_intel.c	11 Sep 2008 20:57:04 -0000
 @@ -252,6 +252,8 @@ ata_raid_read_config_intel(struct wd_sof
  	    aai->aai_capacity / (aai->aai_heads * aai->aai_sectors);
  	aai->aai_offset = map->offset;
  	aai->aai_reserved = 3;
 +	if (map->name)
 +		strlcpy(aai->aai_name, map->name, sizeof(aai->aai_name));

  	/* Fill in disk info */
  	adi = &aai->aai_disks[curdrive];
 Index: sys/dev/ata/ata_raid_jmicron.c
 ===================================================================
 RCS file: /cvsroot/src/sys/dev/ata/ata_raid_jmicron.c,v
 retrieving revision 1.2
 diff -b -u -p -r1.2 ata_raid_jmicron.c
 --- sys/dev/ata/ata_raid_jmicron.c	10 Sep 2008 16:59:32 -0000	1.2
 +++ sys/dev/ata/ata_raid_jmicron.c	11 Sep 2008 20:57:04 -0000
 @@ -240,6 +240,8 @@ ata_raid_read_config_jmicron(struct wd_s
  	    aai->aai_capacity / (aai->aai_heads * aai->aai_sectors);
  	aai->aai_offset = info->offset * 16;
  	aai->aai_reserved = 2;
 +	if (info->name)
 +		strlcpy(aai->aai_name, info->name, sizeof(aai->aai_name));

  	atabus = device_private(device_parent(sc->sc_dev));
  	drive = atabus->sc_chan->ch_channel;
 Index: share/man/man4/ataraid.4
 ===================================================================
 RCS file: /cvsroot/src/share/man/man4/ataraid.4,v
 retrieving revision 1.12
 diff -b -u -p -r1.12 ataraid.4
 --- share/man/man4/ataraid.4	11 Sep 2008 11:18:11 -0000	1.12
 +++ share/man/man4/ataraid.4	11 Sep 2008 20:57:04 -0000
 @@ -61,8 +61,14 @@ Promise FastTrak
  .It
  Via V-RAID (found in many VIA-based motherboards)
  .El
 +.Pp
 +Status of the logical disk as well as the disks associated with it,
 +can be viewed through the
 +.Xr bioctl 8
 +utility.
  .Sh SEE ALSO
 -.Xr ld 4
 +.Xr ld 4 ,
 +.Xr bioctl 8
  .Sh HISTORY
  The
  .Nm

From: matthew green <mrg@eterna.com.au>
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org,
    netbsd-bugs@netbsd.org, xtraeme@gmail.com
Subject: re: kern/39514: ataraid(4): basic bio(4) support 
Date: Fri, 12 Sep 2008 10:13:53 +1000


     +	char	aai_name[32];		/* array volume name */

     [ ... ]

     +	if (aai->aai_name)
     +		strlcpy(bv->bv_vendor, aai->aai_name,
     +		    sizeof(bv->bv_vendor));


 this will always be true.

 it seems that you should make aai_name a char * instead
 of an array, and to malloc/free the space for it as
 necessary.  limiting names to 31 chars is going to bite
 someone in the future.



 .mrg.

From: Juan RP <xtraeme@gmail.com>
To: gnats-bugs@netbsd.org
Cc: matthew green <mrg@eterna.com.au>
Subject: Re: kern/39514: ataraid(4): basic bio(4) support
Date: Fri, 12 Sep 2008 04:48:43 +0200

 On Fri, 12 Sep 2008 10:13:53 +1000
 matthew green <mrg@eterna.com.au> wrote:

 >      
 >     +	char	aai_name[32];		/* array volume name */
 > 
 >     [ ... ]
 > 
 >     +	if (aai->aai_name)
 >     +		strlcpy(bv->bv_vendor, aai->aai_name,
 >     +		    sizeof(bv->bv_vendor));
 > 
 > 
 > this will always be true.
 > 
 > it seems that you should make aai_name a char * instead
 > of an array, and to malloc/free the space for it as
 > necessary.  limiting names to 31 chars is going to bite
 > someone in the future.

 Thanks for the comment.

 But what's the point of using mem from heap if bv_vendor is also
 an array of 31 chars?

 Also, I think this size should be enough for most uses. In all
 metadata formats supported the half of this size is used. Take
 a look at ata_raidreg.h.

Responsible-Changed-From-To: kern-bug-people->tron
Responsible-Changed-By: tron@NetBSD.org
Responsible-Changed-When: Mon, 15 Sep 2008 10:58:19 +0000
Responsible-Changed-Why:
I'll handle this PR.


From: Matthias Scheler <tron@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/39514 CVS commit: src
Date: Mon, 15 Sep 2008 11:44:50 +0000 (UTC)

 Module Name:	src
 Committed By:	tron
 Date:		Mon Sep 15 11:44:50 UTC 2008

 Modified Files:
 	src/share/man/man4: ataraid.4
 	src/sys/dev/ata: ata_raid_adaptec.c ata_raid_intel.c ata_raid_jmicron.c
 	    ata_raidvar.h ld_ataraid.c

 Log Message:
 Add support for status reports via bio(4) to ataraid(4).
 The code was contributed by Juan RP in PR kern/39514.


 To generate a diff of this commit:
 cvs rdiff -r1.12 -r1.13 src/share/man/man4/ataraid.4
 cvs rdiff -r1.8 -r1.9 src/sys/dev/ata/ata_raid_adaptec.c \
     src/sys/dev/ata/ata_raidvar.h
 cvs rdiff -r1.1 -r1.2 src/sys/dev/ata/ata_raid_intel.c
 cvs rdiff -r1.2 -r1.3 src/sys/dev/ata/ata_raid_jmicron.c
 cvs rdiff -r1.29 -r1.30 src/sys/dev/ata/ld_ataraid.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Matthias Scheler <tron@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/39514 CVS commit: src/sys/dev/ata
Date: Mon, 15 Sep 2008 11:53:52 +0000 (UTC)

 Module Name:	src
 Committed By:	tron
 Date:		Mon Sep 15 11:53:52 UTC 2008

 Modified Files:
 	src/sys/dev/ata: ld_ataraid.c

 Log Message:
 Only report volume names if they are non empty.
 Patch provided by Juan RP in PR kern/39514.


 To generate a diff of this commit:
 cvs rdiff -r1.30 -r1.31 src/sys/dev/ata/ld_ataraid.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->closed
State-Changed-By: tron@NetBSD.org
State-Changed-When: Mon, 15 Sep 2008 11:56:09 +0000
State-Changed-Why:
I've committed your code changes. Thanks a lot.


>Unformatted:

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2007 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.