Date: Thu, 17 Nov 2011 10:25:02 -0800 From: Maksim Yevmenkin <emax@freebsd.org> To: Alexander Motin <mav@freebsd.org> Cc: current@freebsd.org Subject: Re: [RFC] ahci(4) patch Message-ID: <CAFPOs6rAaM1BNtC4CkW8yoo200wGrCTmn=WrJBCLrieVqN0QTA@mail.gmail.com> In-Reply-To: <4EC54624.4000409@FreeBSD.org> References: <CAFPOs6rU6AKeTNzKhaqxktDb9CN=g-EDMLf2%2Bcyvhq772SymjA@mail.gmail.com> <4EC43ABA.7060407@FreeBSD.org> <CAFPOs6qyC%2B_8CrxyoCpXBOTTXbgo=vozLNSzHhsH4Hw=jgvERg@mail.gmail.com> <4EC4404B.4070008@FreeBSD.org> <CAFPOs6qAXYQzeD6yP0cuayp8VDuZky%2BZAGyux=5__A0EqSH1Dg@mail.gmail.com> <4EC531A4.4000803@FreeBSD.org> <CAFPOs6pnSs0DZODMQOStvQnf_OQKnhOp1YF2_17kMEa2ri7-wg@mail.gmail.com> <4EC53E28.2030609@FreeBSD.org> <4EC54624.4000409@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Nov 17, 2011 at 9:36 AM, Alexander Motin <mav@freebsd.org> wrote:
> On 11/17/11 19:02, Alexander Motin wrote:
>> On 11/17/11 18:35, Maksim Yevmenkin wrote:
>>> On Thu, Nov 17, 2011 at 8:09 AM, Alexander Motin <mav@freebsd.org> wrot=
e:
>>>> On 11/17/11 01:08, Maksim Yevmenkin wrote:
>>>>> On Wed, Nov 16, 2011 at 2:59 PM, Alexander Motin <mav@freebsd.org> wr=
ote:
>>>>>> On 17.11.2011 00:44, Maksim Yevmenkin wrote:
>>>>>>>
>>>>>>> On Wed, Nov 16, 2011 at 2:35 PM, Alexander Motin<mav@freebsd.org> =
=A0wrote:
>>>>>>>>
>>>>>>>> On 16.11.2011 23:59, Maksim Yevmenkin wrote:
>>>>>>>>>
>>>>>>>>> would anyone object to the following ahci(4) patch?
>>>>>>>>>
>>>>>>>>> =3D=3D
>>>>>>>>>
>>>>>>>>> --- ahci.c.orig 2011-11-16 21:35:26.000000000 +0000
>>>>>>>>> +++ ahci.c =A0 =A0 =A02011-11-16 21:35:41.000000000 +0000
>>>>>>>>> @@ -500,7 +500,7 @@
>>>>>>>>> =A0 =A0 =A0 =A0for (unit =3D 0; unit< =A0 =A0ctlr->channels; unit=
++) {
>>>>>>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if ((ctlr->ichannels& =A0 =A0(1<< =
=A0 =A0unit)) =3D=3D 0)
>>>>>>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0continue;
>>>>>>>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 child =3D device_add_child(dev, "ah=
cich", -1);
>>>>>>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 child =3D device_add_child(dev, "ah=
cich", unit);
>>>>>>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (child =3D=3D NULL)
>>>>>>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0device_printf(dev,=
"failed to add channel
>>>>>>>>> device\n");
>>>>>>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0else
>>>>>>>>>
>>>>>>>>> =3D=3D
>>>>>>>>>
>>>>>>>>> the idea is to have "static" numbering for ada(4) disks.
>>>>>>>>
>>>>>>>> I do. The only way I see this useful is if you have BIOS configure=
d for
>>>>>>>> non-hot-swappable disks, in which case you have some AHCI channels
>>>>>>>> disabled,
>>>>>>>> but want to keep numbers of the rest. While I don't like this mode=
in
>>>>>>>> general, especially when it can't be disabled, that patch could be=
useful
>>>>>>>> in
>>>>>>>> these cases. But in other cases, when you have several AHCI contro=
llers,
>>>>>>>> it
>>>>>>>> just wont not work. You will receive error on attempt to create se=
cond
>>>>>>>> ahcich0.
>>>>>>>
>>>>>>> shouldn't achcichX be destroyed when disk is detached/removed/etc.?
>>>>>>
>>>>>> List of implemented AHCI channels to expose as ahcichX set by BIOS i=
n
>>>>>> vendor-specific way, but only during boot and not by many BIOSes. De=
stroying
>>>>>> them on disk detach theoretically possible, but IMHO not right, as b=
us
>>>>>> doesn't disappear on disk disconnect.
>>>>>>
>>>>>>> the particular problem i'm trying to address is disk re-numbering w=
hen
>>>>>>> one of the disks fails/removed/etc. i'm trying to use hints to wire
>>>>>>> disks to controllers/busses. it works perfectly fine with da(4) dis=
ks
>>>>>>> (even hot swappable ones) , but i can not make it to work with ada(=
4)
>>>>>>> disks. i'm perfectly fine to hid this under some sort of option
>>>>>>> (something similar to ATA_STATIC_ID, AHCI_STATIC_ID for example)
>>>>>>
>>>>>> Wiring works for adaX also, unless your BIOS is so "intelligent" to =
report
>>>>>> unconnected ports and not impliemented.. You should just wire CAM bu=
ses to
>>>>>> ahcichX, not to ahciX. It could be done in other way changing just b=
y one
>>>>>> line around xpt_bus_register(), but now it is done so.
>>>>>
>>>>> ok. then i must be missing something, here is what i have in device.h=
ints
>>>>>
>>>>> hint.scbus.0.at=3D"umass-sim0"
>>>>> hint.scbus.1.at=3D"ahcich0"
>>>>> hint.scbus.2.at=3D"ahcich1"
>>>>> hint.scbus.3.at=3D"ahcich2"
>>>>> hint.scbus.4.at=3D"ahcich3"
>>>>> hint.scbus.5.at=3D"ahcich4"
>>>>> hint.scbus.6.at=3D"ahcich5"
>>>>>
>>>>> hint.da.0.at=3D"scbus0"
>>>>> hint.ada.0.at=3D"scbus1"
>>>>> hint.ada.1.at=3D"scbus2"
>>>>> hint.ada.2.at=3D"scbus3"
>>>>> hint.ada.3.at=3D"scbus4"
>>>>> hint.ada.4.at=3D"scbus5"
>>>>> hint.ada.5.at=3D"scbus6"
>>>>>
>>>>> this is for 6-port ahci(4) compatible controller (intel) on the
>>>>> motherboard. no matter which achi(4) ports are connected, resulted
>>>>> adaX devices are always sequential starting from ada0. so, the
>>>>> question is: what am i doing wrong here?
>>>>
>>>> Just put your lines into my loader.conf and got this:
>>>>
>>>> %camcontrol devlist -v
>>>> scbus1 on ahcich0 bus 0:
>>>> <INTEL SSDSA2M080G2GC 2CV102M3> =A0 =A0at scbus1 target 0 lun 0 (pass0=
,ada0)
>>>> <> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 at =
scbus1 target -1 lun -1 ()
>>>> scbus2 on ahcich1 bus 0:
>>>> <> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 at =
scbus2 target -1 lun -1 ()
>>>> scbus3 on ahcich2 bus 0:
>>>> <INTEL SSDSA2M080G2GC 2CV102M3> =A0 =A0at scbus3 target 0 lun 0 (pass1=
,ada2)
>>>> <> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 at =
scbus3 target -1 lun -1 ()
>>>> scbus4 on ahcich3 bus 0:
>>>> <> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 at =
scbus4 target -1 lun -1 ()
>>>> scbus5 on ahcich4 bus 0:
>>>> <> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 at =
scbus5 target -1 lun -1 ()
>>>> scbus6 on ahcich5 bus 0:
>>>> <> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 at =
scbus6 target -1 lun -1 ()
>>>> ...
>>>>
>>>> I see no problem. What am I doing wrong?
>>>>
>>>> Let me repeat question not asked directly: Does your BIOS reports unus=
ed
>>>> AHCI channels as implemented? Do you always have all 6 ahcich devices =
or
>>>> not?
>>>
>>> i not exactly sure. below is the relevant dmesg. as far as i can tell,
>>> channels are correct, however, ahcich numbering is sequential and that
>>> is why hints dont work.
>>>
>>> Nov 17 09:22:51 PREPROD-red1 kernel: ahci0: <Intel Cougar Point AHCI
>>> SATA controller> port
>>> 0xf070-0xf077,0xf060-0xf063,0xf050-0xf057,0xf040-0xf043,0xf000-0xf01f
>>> mem 0xfbb21000-0xfbb217ff irq 19 at device 31.2 on pci0
>>> Nov 17 09:22:51 PREPROD-red1 kernel: ahci0: attempting to allocate 1
>>> MSI vectors (1 supported)
>>> Nov 17 09:22:51 PREPROD-red1 kernel: ahci0: using IRQ 270 for MSI
>>> Nov 17 09:22:51 PREPROD-red1 kernel: ahci0: AHCI v1.30 with 6 6Gbps
>>> ports, Port Multiplier not supported
>>> Nov 17 09:22:51 PREPROD-red1 kernel: ahci0: Caps: 64bit NCQ SNTF AL
>>> CLO 6Gbps PMD SSC PSC 32cmd EM 6ports
>>> Nov 17 09:22:51 PREPROD-red1 kernel: ahci0: Caps2: APST
>>> Nov 17 09:22:51 PREPROD-red1 kernel: ahci0: EM Caps: ALHD XMT SMB LED
>>> Nov 17 09:22:51 PREPROD-red1 kernel: ahcich0: <AHCI channel> at
>>> channel 0 on ahci0
>>> Nov 17 09:22:51 PREPROD-red1 kernel: ahcich0: Caps:
>>> Nov 17 09:22:51 PREPROD-red1 kernel: ahcich1: <AHCI channel> at
>>> channel 2 on ahci0
>>> Nov 17 09:22:51 PREPROD-red1 kernel: ahcich1: Caps:
>>> Nov 17 09:22:51 PREPROD-red1 kernel: ahcich2: <AHCI channel> at
>>> channel 5 on ahci0
>>> Nov 17 09:22:51 PREPROD-red1 kernel: ahcich2: Caps:
>>
>> Yes, that's it. You should have six ahcichX devices, but have only three
>> - only for connected devices. Some vendors (Supermicro was first I've
>> heard about) last time randomly started to make BIOS mark unused AHCI
>> channels as unimplemented. Sometimes it can be configured in BIOS setup
>> (it may be called like "hot-swappable"), sometimes not. I believe that
>> is wrong AHCI spec interpretation.
>>
>> If it is not configurable in BIOS, we could add respective hint to the
>> ahci(4) to allow user ignore these fake "implemented" flags to always
>> present all possible ports, but it is direct AHCI spec violation.
>>
>> Other possible way is to leave ahcichX device as-is, but instead
>> register ports in CAM in different fashion -- as buses of the same
>> controller. Patch may look like this:
>>
>> --- ahci.c =A0 =A0 =A0(revision 227580)
>> +++ ahci.c =A0 =A0 =A0(working copy)
>> @@ -985,8 +985,8 @@
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err1;
>> =A0 =A0 =A0 =A0 }
>> =A0 =A0 =A0 =A0 /* Construct SIM entry */
>> - =A0 =A0 =A0 ch->sim =3D cam_sim_alloc(ahciaction, ahcipoll, "ahcich", =
ch,
>> - =A0 =A0 =A0 =A0 =A0 device_get_unit(dev), &ch->mtx,
>> + =A0 =A0 =A0 ch->sim =3D cam_sim_alloc(ahciaction, ahcipoll, "ahci", ch=
,
>> + =A0 =A0 =A0 =A0 =A0 device_get_unit(device_get_parent(dev)), &ch->mtx,
>> =A0 =A0 =A0 =A0 =A0 =A0 min(2, ch->numslots),
>> =A0 =A0 =A0 =A0 =A0 =A0 (ch->caps & AHCI_CAP_SNCQ) ? ch->numslots : 0,
>> =A0 =A0 =A0 =A0 =A0 =A0 devq);
>> @@ -996,7 +996,7 @@
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 error =3D ENOMEM;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err1;
>> =A0 =A0 =A0 =A0 }
>> - =A0 =A0 =A0 if (xpt_bus_register(ch->sim, dev, 0) !=3D CAM_SUCCESS) {
>> + =A0 =A0 =A0 if (xpt_bus_register(ch->sim, dev, ch->unit) !=3D CAM_SUCC=
ESS) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 device_printf(dev, "unable to register x=
pt bus\n");
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 error =3D ENXIO;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err2;
>>
>> Then output looks that way:
>> %camcontrol devlist -v
>> scbus1 on ahci0 bus 0:
>> <INTEL SSDSA2M080G2GC 2CV102M3> =A0 =A0at scbus1 target 0 lun 0 (pass0,a=
da0)
>> <> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 at sc=
bus1 target -1 lun -1 ()
>> scbus2 on ahci0 bus 1:
>> <> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 at sc=
bus2 target -1 lun -1 ()
>> scbus3 on ahci0 bus 2:
>> <INTEL SSDSA2M080G2GC 2CV102M3> =A0 =A0at scbus3 target 0 lun 0 (pass1,a=
da2)
>> <> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 at sc=
bus3 target -1 lun -1 ()
>> scbus4 on ahci0 bus 3:
>> <> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 at sc=
bus4 target -1 lun -1 ()
>> scbus5 on ahci0 bus 4:
>> <> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 at sc=
bus5 target -1 lun -1 ()
>> scbus6 on ahci0 bus 5:
>> <> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 at sc=
bus6 target -1 lun -1 ()
>>
>> But in that case two problems remain:
>> =A0- per-port device hints still won't be usable, while changing them wi=
ll
>> break POLA;
>> =A0- it will also break POLA for users who are using wiring now.
>
> One more idea. We can create ahcichX devices for every AHCI channel, but
> disable them for channels marked as not implemented:
>
> --- ahci.c =A0 =A0 =A0(revision 227580)
> +++ ahci.c =A0 =A0 =A0(working copy)
> @@ -498,13 +498,14 @@
> =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0/* Attach all channels on this controller */
> =A0 =A0 =A0 =A0for (unit =3D 0; unit < ctlr->channels; unit++) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ((ctlr->ichannels & (1 << unit)) =3D=3D =
0)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 continue;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0child =3D device_add_child(dev, "ahcich", =
-1);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (child =3D=3D NULL)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (child =3D=3D NULL) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0device_printf(dev, "failed=
to add channel
> device\n");
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 else
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 device_set_ivars(child, (vo=
id *)(intptr_t)unit);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 continue;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 device_set_ivars(child, (void *)(intptr_t)u=
nit);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ((ctlr->ichannels & (1 << unit)) =3D=3D =
0)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 device_disable(child);
> =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0bus_generic_attach(dev);
> =A0 =A0 =A0 =A0return 0;
>
> Device disabling is not very used functionality now, but it seems fit
> perfectly for this case:
>
> ahcich0: <AHCI channel> at channel 0 on ahci0
> ahcich0: Caps: HPCP
> ahcich1: not probed (disabled)
> ahcich2: <AHCI channel> at channel 2 on ahci0
> ahcich2: Caps: HPCP
> ahcich3: <AHCI channel> at channel 3 on ahci0
> ahcich3: Caps: HPCP
> ahcich4: not probed (disabled)
> ahcich5: not probed (disabled)
it looks fine as long as i can wire ada(4) disks to prevent re-ordering
thanks
max
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAFPOs6rAaM1BNtC4CkW8yoo200wGrCTmn=WrJBCLrieVqN0QTA>