On Sun, Dec 29, 2019 at 07:40:01AM +0100, Klemens Nanni wrote:
> On Sun, Dec 29, 2019 at 03:30:15AM +0100, Klemens Nanni wrote:
> > > link->target isn't the right place to put this, for one thing it's only
> > > 16 bits
> > > and the wwn is 64 bits, and it's used throughout the driver to look up
> > > devices
> > > in an array, so changing it will break things. I think link->port_wwn is
> > > the
> > > right place to store it.
> > Ah, link->target was there because I played with that in earlier diffs,
> > port_wwn is indeed correct here.
> >
> > > The fields in the page returned by the driver are also little endian, so
> > > you'd
> > > need letoh64(vpg.wwid) here.
> > Thanks, I see that is done elsewhere in the driver, too.
> >
> > > You should still return 0 here, as continuing on will send the SAS device
> > > page 0 request to the raid volume, which will probably upset the
> > > controller
> > > enough to stop talking to you.
> > Oh well... that explains the hangs - I accidentially dropped the return,
> > it should obviously stay and my diff should only add another page fetch.
> All three points are addressed in the updated diff below, diff three to
> find the root device.
>
> mpii(4) currently leaves the port WWN empty for RAID volumes (physical
> devices are populated). autoboot(9) uses this to match the root device
> as per the second diff, so we must fill in after fetching the relevant
> page as suggested by mikeb and jmatthew.
>
> Feedback? OK?
ok jmatthew@ with one tweak below.
>
> Index: mpii.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/pci/mpii.c,v
> retrieving revision 1.122
> diff -u -p -r1.122 mpii.c
> --- mpii.c 28 Dec 2019 04:38:22 -0000 1.122
> +++ mpii.c 29 Dec 2019 05:02:38 -0000
> @@ -910,8 +910,28 @@ mpii_scsi_probe(struct scsi_link *link)
> if (ISSET(flags, MPII_DF_HIDDEN) || ISSET(flags, MPII_DF_UNUSED))
> return (1);
>
> - if (ISSET(flags, MPII_DF_VOLUME))
> + if (ISSET(flags, MPII_DF_VOLUME)) {
> + struct mpii_cfg_hdr hdr;
> + struct mpii_cfg_raid_vol_pg1 vpg;
> + size_t pagelen;
> +
> + address = MPII_CFG_RAID_VOL_ADDR_HANDLE | dev->dev_handle;
> +
> + if (mpii_req_cfg_header(sc, MPII_CONFIG_REQ_PAGE_TYPE_RAID_VOL,
> + 1, address, MPII_PG_POLL, &hdr) != 0)
> + return (EINVAL);
> +
> + memset(&vpg, 0, sizeof(vpg));
> + pagelen = hdr.page_length * 4;
I'd prefer this:
pagelen = min(hdr.page_length * 4, sizeof(vpg));
just to avoid trashing the stack if the page grows in newer firmware versions.
> +
> + if (mpii_req_cfg_page(sc, address, MPII_PG_POLL, &hdr, 1,
> + &vpg, pagelen) != 0)
> + return (EINVAL);
> +
> + link->port_wwn = letoh64(vpg.wwid);
> +
> return (0);
> + }
>
> memset(&ehdr, 0, sizeof(ehdr));
> ehdr.page_type = MPII_CONFIG_REQ_PAGE_TYPE_EXTENDED;
