Re: request for test: mfii

[Hrvoje Popovski]

On 26.10.2016. 5:50, YASUOKA Masahiko wrote:
> On Wed, 26 Oct 2016 10:26:19 +1100
> Jonathan Gray  wrote:
>> On Tue, Oct 25, 2016 at 05:29:55PM +0900, YASUOKA Masahiko wrote:
>>> I'm working on making mfii(4) bio(4) capable.
>>>
>>> If you have a machine which has mfii(4), I'd like you to test the diff
>>> following.  (It might be risky for production machines for this
>>> moment.)
>>>
>>> After the diff applied, bioctl(8) against the disk (eg. sd0) starts
>>> working and also "sysctl hw.sensors.mfii0" will appear.
>>>
>>> Especially if you can configure a hotspare, testing it is very
>>> helpful for me since I can't use a hotspare on my test machine.
> (snip)
>>> +   case BIOC_SATEST:
>>> +   cmd = MR_DCMD_SPEAKER_TEST;
>>> +   break;
>>> +   default:
>>> +   return (EINVAL);
>>> +   }
>>> +
>>> +   ccb = scsi_io_get(>sc_iopool, 0);
>>> +   rv = mfii_mgmt(sc, ccb, MR_DCMD_PD_SET_STATE, NULL,
>>> +   , sizeof(spkr), flags | SCSI_NOSLEEP);
>>
>> Should this be cmd rather than MR_DCMD_PD_SET_STATE?
>> The cmd values from the switch statement are not used.
> 
> Oops.  Yes, that's right.
> 
>>> +int
>>> +mfii_ioctl_blink(struct mfii_softc *sc, struct bioc_blink *bb)
> (snip)
>>> +   case BIOC_SBBLINK:
>>> +   case BIOC_SBALARM:
>>> +   cmd = MR_DCMD_PD_BLINK;
>>> +   break;
>>> +   default:
>>> +   rv = EINVAL;
>>> +   goto done;
>>> +   }
>>> +
>>> +   ccb = scsi_io_get(>sc_iopool, 0);
>>> +   rv = mfii_mgmt(sc, ccb, cmd, NULL, NULL, 0, SCSI_NOSLEEP);
>>> +   scsi_io_put(>sc_iopool, ccb);
> 
> Passing the mbox to mfii_mgmt() was missing.
> 
>>> + done:
>>> +   free(list, M_TEMP, sizeof(*list));
>>> +
>>> +   return (ENOTTY);
>>> +}
>>
>> Shouldn't this be return (rv) to return the EINVAL values?
>> With rv set to 0 before the 'done' to return 0 when there is no error?
> 
> Yes, that's also right.  Thanks.


Hi,

with this version boot stops here:

mfii0 at pci7 dev 0 function 0 "Symbios Logic MegaRAID SAS2208" rev
0x05: msi
mfii0: "ServeRAID M5110", firmware 23.34.0-0016, 512MB cache
scsibus1 at mfii0: 64 targets
sd0 at scsibus1 targ 0 lun 0:  SCSI3
0/direct fixed naa.600605b006c3a0b01a582bd010e4c053
sd0: 952720MB, 512 bytes/sector, 1951170560 sectors
mfii0: timeout on ccb 1008
mfii_mgmt cmd=50593792 failed status=255

Re: request for test: mfii

[Johan Huldtgren]

On 10/25/16 23:50, YASUOKA Masahiko wrote:
> On Wed, 26 Oct 2016 10:26:19 +1100
> Jonathan Gray  wrote:
>> On Tue, Oct 25, 2016 at 05:29:55PM +0900, YASUOKA Masahiko wrote:
>>> I'm working on making mfii(4) bio(4) capable.
>>>
>>> If you have a machine which has mfii(4), I'd like you to test the diff
>>> following.  (It might be risky for production machines for this
>>> moment.)
>>>
>>> After the diff applied, bioctl(8) against the disk (eg. sd0) starts
>>> working and also "sysctl hw.sensors.mfii0" will appear.
>>>
>>> Especially if you can configure a hotspare, testing it is very
>>> helpful for me since I can't use a hotspare on my test machine.
> (snip)
>>> +   case BIOC_SATEST:
>>> +   cmd = MR_DCMD_SPEAKER_TEST;
>>> +   break;
>>> +   default:
>>> +   return (EINVAL);
>>> +   }
>>> +
>>> +   ccb = scsi_io_get(>sc_iopool, 0);
>>> +   rv = mfii_mgmt(sc, ccb, MR_DCMD_PD_SET_STATE, NULL,
>>> +   , sizeof(spkr), flags | SCSI_NOSLEEP);
>>
>> Should this be cmd rather than MR_DCMD_PD_SET_STATE?
>> The cmd values from the switch statement are not used.
> 
> Oops.  Yes, that's right.
> 
>>> +int
>>> +mfii_ioctl_blink(struct mfii_softc *sc, struct bioc_blink *bb)
> (snip)
>>> +   case BIOC_SBBLINK:
>>> +   case BIOC_SBALARM:
>>> +   cmd = MR_DCMD_PD_BLINK;
>>> +   break;
>>> +   default:
>>> +   rv = EINVAL;
>>> +   goto done;
>>> +   }
>>> +
>>> +   ccb = scsi_io_get(>sc_iopool, 0);
>>> +   rv = mfii_mgmt(sc, ccb, cmd, NULL, NULL, 0, SCSI_NOSLEEP);
>>> +   scsi_io_put(>sc_iopool, ccb);
> 
> Passing the mbox to mfii_mgmt() was missing.
> 
>>> + done:
>>> +   free(list, M_TEMP, sizeof(*list));
>>> +
>>> +   return (ENOTTY);
>>> +}
>>
>> Shouldn't this be return (rv) to return the EINVAL values?
>> With rv set to 0 before the 'done' to return 0 when there is no error?
> 
> Yes, that's also right.  Thanks.
> 
> Let me update the diff.
> 
> Index: sys/dev/pci/mfii.c
> ===
> RCS file: /cvs/src/sys/dev/pci/mfii.c,v
> retrieving revision 1.28
> diff -u -p -r1.28 mfii.c
> --- sys/dev/pci/mfii.c24 Oct 2016 05:27:52 -  1.28
> +++ sys/dev/pci/mfii.c26 Oct 2016 03:46:15 -
> @@ -22,9 +22,11 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> +#include 
>  #include 
>  #include 
>  
> @@ -212,6 +214,13 @@ struct mfii_iop {
>   u_int8_t sge_flag_eol;
>  };
>  
> +struct mfii_cfg {
> + struct mfi_conf *cfg;
> + struct mfi_array*cfg_array;
> + struct mfi_ld_cfg   *cfg_ld;
> + struct mfi_hotspare *cfg_hs;
> +};
> +
>  struct mfii_softc {
>   struct device   sc_dev;
>   const struct mfii_iop   *sc_iop;
> @@ -250,11 +259,15 @@ struct mfii_softc {
>   struct scsi_iopool  sc_iopool;
>  
>   struct mfi_ctrl_infosc_info;
> +
> + struct ksensor  *sc_sensors;
> + struct ksensordev   sc_sensordev;
>  };
>  
>  int  mfii_match(struct device *, void *, void *);
>  void mfii_attach(struct device *, struct device *, void *);
>  int  mfii_detach(struct device *, int);
> +int  mfii_scsi_ioctl(struct scsi_link *, u_long, caddr_t, int);
>  
>  struct cfattach mfii_ca = {
>   sizeof(struct mfii_softc),
> @@ -277,7 +290,7 @@ struct scsi_adapter mfii_switch = {
>   scsi_minphys,
>   NULL, /* probe */
>   NULL, /* unprobe */
> - NULL  /* ioctl */
> + mfii_scsi_ioctl
>  };
>  
>  void mfii_pd_scsi_cmd(struct scsi_xfer *);
> @@ -334,7 +347,26 @@ int  mfii_scsi_cmd_cdb(struct 
> mfii_soft
>   struct scsi_xfer *);
>  int  mfii_pd_scsi_cmd_cdb(struct mfii_softc *,
>   struct scsi_xfer *);
> -
> +int  mfii_scsi_ioctl_cache(struct scsi_link *, u_int,
> + struct dk_cache *);
> +#if NBIO > 0
> +int  mfii_ioctl(struct device *, u_long, caddr_t);
> +int  mfii_fill_cfg(struct mfii_softc *, struct mfii_cfg *);
> +int  mfii_ioctl_inq(struct mfii_softc *, struct bioc_inq *);
> +int  mfii_ioctl_vol(struct mfii_softc *, struct bioc_vol *);
> +int  mfii_ioctl_disk(struct mfii_softc *,
> + struct bioc_disk *);
> +int  mfii_ioctl_alarm(struct mfii_softc *,
> + struct bioc_alarm *);
> +int  mfii_ioctl_blink(struct mfii_softc *,
> + struct bioc_blink *);
> +int  mfii_ioctl_setstate(struct mfii_softc *,
> + struct bioc_setstate *);
> +int  mfii_ioctl_patrol(struct mfii_softc *,
> + struct bioc_patrol *);
> +int  mfii_create_sensors(struct mfii_softc *);
> +void mfii_refresh_sensors(void *);
> +#endif

Re: request for test: mfii

[YASUOKA Masahiko]

On Wed, 26 Oct 2016 10:26:19 +1100
Jonathan Gray  wrote:
> On Tue, Oct 25, 2016 at 05:29:55PM +0900, YASUOKA Masahiko wrote:
>> I'm working on making mfii(4) bio(4) capable.
>> 
>> If you have a machine which has mfii(4), I'd like you to test the diff
>> following.  (It might be risky for production machines for this
>> moment.)
>> 
>> After the diff applied, bioctl(8) against the disk (eg. sd0) starts
>> working and also "sysctl hw.sensors.mfii0" will appear.
>> 
>> Especially if you can configure a hotspare, testing it is very
>> helpful for me since I can't use a hotspare on my test machine.
(snip)
>> +case BIOC_SATEST:
>> +cmd = MR_DCMD_SPEAKER_TEST;
>> +break;
>> +default:
>> +return (EINVAL);
>> +}
>> +
>> +ccb = scsi_io_get(>sc_iopool, 0);
>> +rv = mfii_mgmt(sc, ccb, MR_DCMD_PD_SET_STATE, NULL,
>> +, sizeof(spkr), flags | SCSI_NOSLEEP);
> 
> Should this be cmd rather than MR_DCMD_PD_SET_STATE?
> The cmd values from the switch statement are not used.

Oops.  Yes, that's right.

>> +int
>> +mfii_ioctl_blink(struct mfii_softc *sc, struct bioc_blink *bb)
(snip)
>> +case BIOC_SBBLINK:
>> +case BIOC_SBALARM:
>> +cmd = MR_DCMD_PD_BLINK;
>> +break;
>> +default:
>> +rv = EINVAL;
>> +goto done;
>> +}
>> +
>> +ccb = scsi_io_get(>sc_iopool, 0);
>> +rv = mfii_mgmt(sc, ccb, cmd, NULL, NULL, 0, SCSI_NOSLEEP);
>> +scsi_io_put(>sc_iopool, ccb);

Passing the mbox to mfii_mgmt() was missing.

>> + done:
>> +free(list, M_TEMP, sizeof(*list));
>> +
>> +return (ENOTTY);
>> +}
> 
> Shouldn't this be return (rv) to return the EINVAL values?
> With rv set to 0 before the 'done' to return 0 when there is no error?

Yes, that's also right.  Thanks.

Let me update the diff.

Index: sys/dev/pci/mfii.c
===
RCS file: /cvs/src/sys/dev/pci/mfii.c,v
retrieving revision 1.28
diff -u -p -r1.28 mfii.c
--- sys/dev/pci/mfii.c  24 Oct 2016 05:27:52 -  1.28
+++ sys/dev/pci/mfii.c  26 Oct 2016 03:46:15 -
@@ -22,9 +22,11 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
+#include 
 #include 
 #include 
 
@@ -212,6 +214,13 @@ struct mfii_iop {
u_int8_t sge_flag_eol;
 };
 
+struct mfii_cfg {
+   struct mfi_conf *cfg;
+   struct mfi_array*cfg_array;
+   struct mfi_ld_cfg   *cfg_ld;
+   struct mfi_hotspare *cfg_hs;
+};
+
 struct mfii_softc {
struct device   sc_dev;
const struct mfii_iop   *sc_iop;
@@ -250,11 +259,15 @@ struct mfii_softc {
struct scsi_iopool  sc_iopool;
 
struct mfi_ctrl_infosc_info;
+
+   struct ksensor  *sc_sensors;
+   struct ksensordev   sc_sensordev;
 };
 
 intmfii_match(struct device *, void *, void *);
 void   mfii_attach(struct device *, struct device *, void *);
 intmfii_detach(struct device *, int);
+intmfii_scsi_ioctl(struct scsi_link *, u_long, caddr_t, int);
 
 struct cfattach mfii_ca = {
sizeof(struct mfii_softc),
@@ -277,7 +290,7 @@ struct scsi_adapter mfii_switch = {
scsi_minphys,
NULL, /* probe */
NULL, /* unprobe */
-   NULL  /* ioctl */
+   mfii_scsi_ioctl
 };
 
 void   mfii_pd_scsi_cmd(struct scsi_xfer *);
@@ -334,7 +347,26 @@ intmfii_scsi_cmd_cdb(struct 
mfii_soft
struct scsi_xfer *);
 intmfii_pd_scsi_cmd_cdb(struct mfii_softc *,
struct scsi_xfer *);
-
+intmfii_scsi_ioctl_cache(struct scsi_link *, u_int,
+   struct dk_cache *);
+#if NBIO > 0
+intmfii_ioctl(struct device *, u_long, caddr_t);
+intmfii_fill_cfg(struct mfii_softc *, struct mfii_cfg *);
+intmfii_ioctl_inq(struct mfii_softc *, struct bioc_inq *);
+intmfii_ioctl_vol(struct mfii_softc *, struct bioc_vol *);
+intmfii_ioctl_disk(struct mfii_softc *,
+   struct bioc_disk *);
+intmfii_ioctl_alarm(struct mfii_softc *,
+   struct bioc_alarm *);
+intmfii_ioctl_blink(struct mfii_softc *,
+   struct bioc_blink *);
+intmfii_ioctl_setstate(struct mfii_softc *,
+   struct bioc_setstate *);
+intmfii_ioctl_patrol(struct mfii_softc *,
+   struct bioc_patrol *);
+intmfii_create_sensors(struct mfii_softc *);
+void   mfii_refresh_sensors(void *);
+#endif
 
 #define mfii_fw_state(_sc) mfii_read((_sc), MFI_OSP)
 
@@ -506,7 +538,8 @@ mfii_attach(struct device *parent, struc
memset(, 0, sizeof(saa));
saa.saa_sc_link = 

Re: request for test: mfii

[Jonathan Gray]

On Tue, Oct 25, 2016 at 05:29:55PM +0900, YASUOKA Masahiko wrote:
> Hi,
> 
> I'm working on making mfii(4) bio(4) capable.
> 
> If you have a machine which has mfii(4), I'd like you to test the diff
> following.  (It might be risky for production machines for this
> moment.)
> 
> After the diff applied, bioctl(8) against the disk (eg. sd0) starts
> working and also "sysctl hw.sensors.mfii0" will appear.
> 
> Especially if you can configure a hotspare, testing it is very
> helpful for me since I can't use a hotspare on my test machine.

> +
> +int
> +mfii_ioctl_alarm(struct mfii_softc *sc, struct bioc_alarm *ba)
> +{
> + struct mfii_ccb *ccb;
> + u_char spkr;
> + int rv, cmd, flags = 0;
> +
> + if (!ISSET(letoh32(sc->sc_info.mci_hw_present), MFI_INFO_HW_ALARM))
> + return (ENXIO);
> +
> + switch (ba->ba_status) {
> + case BIOC_SADISABLE:
> + cmd = MR_DCMD_SPEAKER_DISABLE;
> + break;
> + case BIOC_SAENABLE:
> + cmd = MR_DCMD_SPEAKER_ENABLE;
> + break;
> + case BIOC_SASILENCE:
> + cmd = MR_DCMD_SPEAKER_SILENCE;
> + break;
> + case BIOC_GASTATUS:
> + cmd = MR_DCMD_SPEAKER_GET;
> + flags = SCSI_DATA_IN;
> + break;
> + case BIOC_SATEST:
> + cmd = MR_DCMD_SPEAKER_TEST;
> + break;
> + default:
> + return (EINVAL);
> + }
> +
> + ccb = scsi_io_get(>sc_iopool, 0);
> + rv = mfii_mgmt(sc, ccb, MR_DCMD_PD_SET_STATE, NULL,
> + , sizeof(spkr), flags | SCSI_NOSLEEP);

Should this be cmd rather than MR_DCMD_PD_SET_STATE?
The cmd values from the switch statement are not used.

> + scsi_io_put(>sc_iopool, ccb);
> + if (rv != 0)
> + return (rv);
> +
> + ba->ba_status = (ba->ba_status == BIOC_GASTATUS)? spkr : 0;
> +
> + return (rv);
> +}
> +
> +int
> +mfii_ioctl_blink(struct mfii_softc *sc, struct bioc_blink *bb)
> +{
> + struct mfi_pd_list *list = NULL;
> + struct mfii_ccb *ccb;
> + uint8_t mbox[MFI_MBOX_SIZE];
> + int rv, i, cmd;
> +
> + list = malloc(sizeof(*list), M_TEMP, M_WAITOK | M_ZERO);
> +
> + ccb = scsi_io_get(>sc_iopool, 0);
> + rv = mfii_mgmt(sc, ccb, MR_DCMD_PD_GET_LIST, NULL,
> + list, sizeof(*list), SCSI_DATA_IN | SCSI_NOSLEEP);
> + scsi_io_put(>sc_iopool, ccb);
> + if (rv != 0)
> + goto done;
> +
> + for (i = 0; i < letoh16(list->mpl_no_pd); i++)
> + if (list->mpl_address[i].mpa_enc_index == bb->bb_channel &&
> + list->mpl_address[i].mpa_enc_slot == bb->bb_target)
> + break;
> + if (i >= letoh16(list->mpl_no_pd)) {
> + rv = EINVAL;
> + goto done;
> + }
> +
> + memset(mbox, 0, sizeof(mbox));
> + *((uint16_t *)[0]) = list->mpl_address[i].mpa_pd_id;
> +
> + switch (bb->bb_status) {
> + case BIOC_SBUNBLINK:
> + cmd = MR_DCMD_PD_UNBLINK;
> + break;
> + case BIOC_SBBLINK:
> + case BIOC_SBALARM:
> + cmd = MR_DCMD_PD_BLINK;
> + break;
> + default:
> + rv = EINVAL;
> + goto done;
> + }
> +
> + ccb = scsi_io_get(>sc_iopool, 0);
> + rv = mfii_mgmt(sc, ccb, cmd, NULL, NULL, 0, SCSI_NOSLEEP);
> + scsi_io_put(>sc_iopool, ccb);
> +
> + done:
> + free(list, M_TEMP, sizeof(*list));
> +
> + return (ENOTTY);
> +}

Shouldn't this be return (rv) to return the EINVAL values?
With rv set to 0 before the 'done' to return 0 when there is no error?