On Fri, 2009-07-17 at 16:20 +0800, jack wang wrote:
> Hi, James
> As your comments ,we resend this patch again. All comments & style issue
> have fixed.
> Best regards!
This looks much better thanks.
It still could do with a description; I created one this time, but I
can't keep on doing that.
> Jack Wang
> >From 2b269e6d73fae3a7c83171e97d894fc84798c35a Mon Sep 17 00:00:00 2001
> From: Tom Peng <tom_peng@xxxxxxxxx>
> Date: Fri, 17 Jul 2009 16:02:04 +0800
> Subject: [PATCH] fix libsas wide port hotplug issue v2
>
> Signed-off-by: Tom Peng <tom_peng@xxxxxxxxx>
> Signed-off-by: Jack Wang <jack_wang@xxxxxxxxx>
> Signed-off-by: Lindar Liu <lindar_liu@xxxxxxxxx>
> Signed-off-by: Kevin Ao <aoqingyun@xxxxxxxxx>
> ---
> drivers/scsi/libsas/sas_expander.c | 149
> ++++++++++++++++++++++++++----------
> 1 files changed, 108 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/scsi/libsas/sas_expander.c
> b/drivers/scsi/libsas/sas_expander.c
> index 54fa1e4..24d108f 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -766,6 +766,7 @@ static int sas_ex_join_wide_port(struct domain_device
> *parent, int phy_id)
> if (!memcmp(phy->attached_sas_addr, ephy->attached_sas_addr,
> SAS_ADDR_SIZE) && ephy->port) {
> sas_port_add_phy(ephy->port, phy->phy);
> + phy->port = ephy->port;
> phy->phy_state = PHY_DEVICE_DISCOVERED;
> return 0;
> }
> @@ -945,10 +946,17 @@ static int sas_ex_discover_dev(struct domain_device
> *dev, int phy_id)
> if (ex->ex_phy[i].phy_state == PHY_VACANT ||
> ex->ex_phy[i].phy_state == PHY_NOT_PRESENT)
> continue;
> -
> + /* in before discover process, it does not add the
> phy
> + * to wide port, so we add the phy to the wide port
> here.
> + */
> if (SAS_ADDR(ex->ex_phy[i].attached_sas_addr) ==
> - SAS_ADDR(child->sas_addr))
> + SAS_ADDR(child->sas_addr)) {
> ex->ex_phy[i].phy_state=
> PHY_DEVICE_DISCOVERED;
> + res = sas_ex_join_wide_port(dev, i);
> + if (!res)
> + SAS_DPRINTK("Add ex phy %d to this "
> + "wide port.\n", i);
For consistency, I made this message the same as the prior one
> + }
And added a missing res = 0 here. Without this, we exit the loop with
res = -ENODEV and then return that to the caller, which is wrong.
> }
> }
>
> @@ -1598,7 +1606,7 @@ static int sas_get_phy_attached_sas_addr(struct
> domain_device *dev,
> }
>
> static int sas_find_bcast_phy(struct domain_device *dev, int *phy_id,
> - int from_phy)
> + int from_phy, bool update)
> {
> struct expander_device *ex = &dev->ex_dev;
> int res = 0;
> @@ -1611,7 +1619,9 @@ static int sas_find_bcast_phy(struct domain_device
> *dev, int *phy_id,
> if (res)
> goto out;
> else if (phy_change_count != ex->ex_phy[i].phy_change_count)
> {
> - ex->ex_phy[i].phy_change_count = phy_change_count;
> + if (update)
> + ex->ex_phy[i].phy_change_count =
> + phy_change_count;
> *phy_id = i;
> return 0;
> }
> @@ -1653,31 +1663,52 @@ out:
> kfree(rg_req);
> return res;
> }
> +/**
> + * sas_find_bcast_dev - find the device issue BROADCAST(CHANGE).
> + * @dev:domain device to be detect.
> + * @src_dev: the device which originated BROADCAST(CHANGE).
> + *
> + * Add self-configuration expander suport. Suppose two expander cascading,
> + * when the first level expander is self-configuring, hotplug the disks in
> + * second level expander, BROADCAST(CHANGE) will not only be originated
> + * in the second level expander, but also be originated in the first level
> + * expander (see SAS protocol SAS 2r-14, 7.11 for detail), it is to say,
> + * expander changed count in two level expanders will all increment at
> least
> + * once, but the phy which chang count has changed is the source device
> which
> + * we concerned.
> + */
>
> static int sas_find_bcast_dev(struct domain_device *dev,
> struct domain_device **src_dev)
> {
> struct expander_device *ex = &dev->ex_dev;
> int ex_change_count = -1;
> - int res;
> + int phy_id = -1;
> + int res, i = 0;
This i=0 is superfluous, you only use it to carry zero into
sas_find_bcast_phy()
> + struct domain_device *ch;
>
> res = sas_get_ex_change_count(dev, &ex_change_count);
> if (res)
> goto out;
> - if (ex_change_count != -1 &&
> - ex_change_count != ex->ex_change_count) {
> - *src_dev = dev;
> - ex->ex_change_count = ex_change_count;
> - } else {
> - struct domain_device *ch;
> -
> - list_for_each_entry(ch, &ex->children, siblings) {
> - if (ch->dev_type == EDGE_DEV ||
> - ch->dev_type == FANOUT_DEV) {
> - res = sas_find_bcast_dev(ch, src_dev);
> - if (src_dev)
> - return res;
> - }
> + if (ex_change_count != -1 && ex_change_count != ex->ex_change_count)
> {
> + /* Just detect if this expander phys phy change count
> changed,
> + * in order to determine if this expander originate
> BROADCAST,
> + * and do not update phy change count field in our structure.
> + */
> + res = sas_find_bcast_phy(dev, &phy_id, i, FALSE);
> + if (phy_id != -1) {
> + *src_dev = dev;
> + ex->ex_change_count = ex_change_count;
> + SAS_DPRINTK("Expander phy change count has changed
> \n");
> + return res;
> + } else
> + SAS_DPRINTK("Expander phys DID NOT changed \n");
> + }
> + list_for_each_entry(ch, &ex->children, siblings) {
> + if (ch->dev_type == EDGE_DEV || ch->dev_type == FANOUT_DEV)
> {
> + res = sas_find_bcast_dev(ch, src_dev);
> + if (src_dev)
> + return res;
> }
> }
> out:
> @@ -1700,24 +1731,26 @@ static void sas_unregister_ex_tree(struct
> domain_device *dev)
> }
>
> static void sas_unregister_devs_sas_addr(struct domain_device *parent,
> - int phy_id)
> + int phy_id, bool last)
> {
> struct expander_device *ex_dev = &parent->ex_dev;
> struct ex_phy *phy = &ex_dev->ex_phy[phy_id];
> struct domain_device *child, *n;
> -
> - list_for_each_entry_safe(child, n, &ex_dev->children, siblings) {
> - if (SAS_ADDR(child->sas_addr) ==
> - SAS_ADDR(phy->attached_sas_addr)) {
> - if (child->dev_type == EDGE_DEV ||
> - child->dev_type == FANOUT_DEV)
> - sas_unregister_ex_tree(child);
> - else
> - sas_unregister_dev(child);
> - break;
> + if (last) {
> + list_for_each_entry_safe(child, n,
> + &ex_dev->children, siblings) {
> + if (SAS_ADDR(child->sas_addr) ==
> + SAS_ADDR(phy->attached_sas_addr)) {
> + if (child->dev_type == EDGE_DEV ||
> + child->dev_type == FANOUT_DEV)
> + sas_unregister_ex_tree(child);
> + else
> + sas_unregister_dev(child);
> + break;
> + }
> }
> + sas_disable_routing(parent, phy->attached_sas_addr);
> }
> - sas_disable_routing(parent, phy->attached_sas_addr);
> memset(phy->attached_sas_addr, 0, SAS_ADDR_SIZE);
> sas_port_delete_phy(phy->port, phy->phy);
> if (phy->port->num_phys == 0)
> @@ -1770,15 +1803,31 @@ static int sas_discover_new(struct domain_device
> *dev, int phy_id)
> {
> struct ex_phy *ex_phy = &dev->ex_dev.ex_phy[phy_id];
> struct domain_device *child;
> - int res;
> + bool found = FALSE;
> + int res, i = 0;
No need to zero initialise i ... it's only used in a for loop where it
gets initialised.
>
> SAS_DPRINTK("ex %016llx phy%d new device attached\n",
> SAS_ADDR(dev->sas_addr), phy_id);
> res = sas_ex_phy_discover(dev, phy_id);
> if (res)
> goto out;
> + /* to support the wide port inserted */
> + for (i = 0; i < dev->ex_dev.num_phys; i++) {
> + struct ex_phy *ex_phy_temp = &dev->ex_dev.ex_phy[i];
> + if (i == phy_id)
> + continue;
> + if (SAS_ADDR(ex_phy_temp->attached_sas_addr) ==
> + SAS_ADDR(ex_phy->attached_sas_addr)) {
> + found = TRUE;
> + break;
> + }
> + }
> + if (found == TRUE) {
> + sas_ex_join_wide_port(dev, phy_id);
> + return 0;
> + }
> res = sas_ex_discover_devices(dev, phy_id);
> - if (res)
> + if (!res)
> goto out;
> list_for_each_entry(child, &dev->ex_dev.children, siblings) {
> if (SAS_ADDR(child->sas_addr) ==
> @@ -1793,7 +1842,8 @@ out:
> return res;
> }
>
> -static int sas_rediscover_dev(struct domain_device *dev, int phy_id)
> +static int sas_rediscover_dev(struct domain_device *dev, int phy_id,
> + bool last)
Should be lined up under the first arg, but actually it will fit on one
line without going over 80 characters.
> {
> struct expander_device *ex = &dev->ex_dev;
> struct ex_phy *phy = &ex->ex_phy[phy_id];
> @@ -1804,11 +1854,11 @@ static int sas_rediscover_dev(struct domain_device
> *dev, int phy_id)
> switch (res) {
> case SMP_RESP_NO_PHY:
> phy->phy_state = PHY_NOT_PRESENT;
> - sas_unregister_devs_sas_addr(dev, phy_id);
> + sas_unregister_devs_sas_addr(dev, phy_id, last);
> goto out; break;
> case SMP_RESP_PHY_VACANT:
> phy->phy_state = PHY_VACANT;
> - sas_unregister_devs_sas_addr(dev, phy_id);
> + sas_unregister_devs_sas_addr(dev, phy_id, last);
> goto out; break;
> case SMP_RESP_FUNC_ACC:
> break;
> @@ -1816,7 +1866,7 @@ static int sas_rediscover_dev(struct domain_device
> *dev, int phy_id)
>
> if (SAS_ADDR(attached_sas_addr) == 0) {
> phy->phy_state = PHY_EMPTY;
> - sas_unregister_devs_sas_addr(dev, phy_id);
> + sas_unregister_devs_sas_addr(dev, phy_id, last);
> } else if (SAS_ADDR(attached_sas_addr) ==
> SAS_ADDR(phy->attached_sas_addr)) {
> SAS_DPRINTK("ex %016llx phy 0x%x broadcast flutter\n",
> @@ -1828,12 +1878,28 @@ out:
> return res;
> }
>
> +/**
> + * sas_rediscover -- revalidate the domain.
single - for docbook, not double.
> + * @dev:domain device to be detect.
> + * @phy_id: the phy id will be detected.
> + * NOTE: this process _must_ quit (return) as soon as any connection
> + * errors are encountered. Connection recovery is done elsewhere.
> + * Discover process only interrogates devices in order to discover the
> + * domain.For plugging out, we un-register the device only when it is
> + * the last phy in the port, for other phys in this port, we just delete it
> + * from the port.For inserting, we do discovery when it is the
> + * first phy,for other phys in this port, we add it to the port to
> + * forming the wide-port.
> + */
> static int sas_rediscover(struct domain_device *dev, const int phy_id)
> {
> struct expander_device *ex = &dev->ex_dev;
> struct ex_phy *changed_phy = &ex->ex_phy[phy_id];
> int res = 0;
> int i;
> + /* To check whether the last phy of a port determining whether or
> not
> + delete devices*/
> + bool last = TRUE;
>
> SAS_DPRINTK("ex %016llx phy%d originated BROADCAST(CHANGE)\n",
> SAS_ADDR(dev->sas_addr), phy_id);
> @@ -1848,13 +1914,13 @@ static int sas_rediscover(struct domain_device *dev,
> const int phy_id)
> SAS_ADDR(changed_phy->attached_sas_addr)) {
> SAS_DPRINTK("phy%d part of wide port with "
> "phy%d\n", phy_id, i);
> - goto out;
> + last = FALSE;
> + break;
> }
> }
> - res = sas_rediscover_dev(dev, phy_id);
> + res = sas_rediscover_dev(dev, phy_id, last);
> } else
> res = sas_discover_new(dev, phy_id);
> -out:
> return res;
> }
>
> @@ -1881,7 +1947,7 @@ int sas_ex_revalidate_domain(struct domain_device
> *port_dev)
>
> do {
> phy_id = -1;
> - res = sas_find_bcast_phy(dev, &phy_id, i);
> + res = sas_find_bcast_phy(dev, &phy_id, i, TRUE);
> if (phy_id == -1)
> break;
> res = sas_rediscover(dev, phy_id);
> @@ -1946,3 +2012,4 @@ int sas_smp_handler(struct Scsi_Host *shost, struct
> sas_rphy *rphy,
>
> return ret;
> }
> +
This is a spurious newline at the end of the file.
James
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
