Re: [RFC PATCH 3/5] scsi: ufs: Introduce HPB module

[Daejun Park]

> > +  if (total_srgn_cnt != 0) {
> > +    dev_err(hba->dev, "ufshpb(%d) error total_subregion_count %d",
> > +      hpb->lun, total_srgn_cnt);
> > +    goto release_srgn_table;
> > +  }
> > +
> > +  return 0;
> > +release_srgn_table:
> > +  for (i = 0; i < rgn_idx; i++) {
> > +    rgn = rgn_table + i;
> > +    if (rgn->srgn_tbl)
> > +      kvfree(rgn->srgn_tbl);
> > +  }

> Please insert a blank line above goto labels as is done in most of the
> kernel code.
OK, I will fix it.

> > +static struct device_attribute ufshpb_sysfs_entries[] = {
> > +  __ATTR(hit_count, 0444, ufshpb_sysfs_show_hit_cnt, NULL),
> > +  __ATTR(miss_count, 0444, ufshpb_sysfs_show_miss_cnt, NULL),
> > +  __ATTR(rb_noti_count, 0444, ufshpb_sysfs_show_rb_noti_cnt, NULL),
> > +  __ATTR(rb_active_count, 0444, ufshpb_sysfs_show_rb_active_cnt, NULL),
> > +  __ATTR(rb_inactive_count, 0444, ufshpb_sysfs_show_rb_inactive_cnt,
> > +         NULL),
> > +  __ATTR(map_req_count, 0444, ufshpb_sysfs_show_map_req_cnt, NULL),
> > +  __ATTR_NULL
> > +};

> Please use __ATTR_RO() where appropriate.
They are only readable attributes. So I changed the code to use __ATTR_RO.

> > +static int ufshpb_create_sysfs(struct ufs_hba *hba, struct ufshpb_lu *hpb)
> > +{
> > +  struct device_attribute *attr;
> > +  int ret;
> > +
> > +  device_initialize(&hpb->hpb_lu_dev);
> > +
> > +  ufshpb_stat_init(hpb);
> > +
> > +  hpb->hpb_lu_dev.parent = get_device(&hba->ufsf.hpb_dev);
> > +  hpb->hpb_lu_dev.release = ufshpb_dev_release;
> > +  dev_set_name(&hpb->hpb_lu_dev, "ufshpb_lu%d", hpb->lun);
> > +
> > +  ret = device_add(&hpb->hpb_lu_dev);
> > +  if (ret) {
> > +    dev_err(hba->dev, "ufshpb(%d) device_add failed",
> > +      hpb->lun);
> > +    return -ENODEV;
> > +  }
> > +
> > +  for (attr = ufshpb_sysfs_entries; attr->attr.name != NULL; attr++) {
> > +    if (device_create_file(&hpb->hpb_lu_dev, attr))
> > +      dev_err(hba->dev, "ufshpb(%d) %s create file error\n",
> > +        hpb->lun, attr->attr.name);
> > +  }
> > +
> > +  return 0;
> > +}

> This is the wrong way to create sysfs attributes. Please set the
> 'groups' member of struct device instead of using a loop to create sysfs
> attributes. The former approach is compatible with udev but the latter
> approach is not.
OK, I changed to create attributes without loop.

> > +static void ufshpb_probe_async(void *data, async_cookie_t cookie)
> > +{
> > +  struct ufshpb_dev_info hpb_dev_info = { 0 };
> > +  struct ufs_hba *hba = data;
> > +  char *desc_buf;
> > +  int ret;
> > +
> > +  desc_buf = kzalloc(QUERY_DESC_MAX_SIZE, GFP_KERNEL);
> > +  if (!desc_buf)
> > +    goto release_desc_buf;
> > +
> > +  ret = ufshpb_get_dev_info(hba, &hpb_dev_info, desc_buf);
> > +  if (ret)
> > +    goto release_desc_buf;
> > +
> > +  /*
> > +   * Because HPB driver uses scsi_device data structure,
> > +   * we should wait at this point until finishing initialization of all
> > +   * scsi devices. Even if timeout occurs, HPB driver will search
> > +   * the scsi_device list on struct scsi_host (shost->__host list_head)
> > +   * and can find out HPB logical units in all scsi_devices
> > +   */
> > +  wait_event_timeout(hba->ufsf.sdev_wait,
> > +         (atomic_read(&hba->ufsf.slave_conf_cnt)
> > +        == hpb_dev_info.num_lu),
> > +         SDEV_WAIT_TIMEOUT);
> > +
> > +  dev_dbg(hba->dev, "ufshpb: slave count %d, lu count %d\n",
> > +    atomic_read(&hba->ufsf.slave_conf_cnt), hpb_dev_info.num_lu);
> > +
> > +  ufshpb_scan_hpb_lu(hba, &hpb_dev_info, desc_buf);
> > +release_desc_buf:
> > +  kfree(desc_buf);
> > +}

> What happens if two LUNs are added before the above code is woken up?
> Will that perhaps cause the wait_event_timeout() call to wait forever?
I don't think it is problem. I think that the wait_event_timeout() will
check the condition before waiting.

> > +static int ufshpb_probe(struct device *dev)
> > +{
> > +  struct ufs_hba *hba;
> > +  struct ufsf_feature_info *ufsf;
> > +
> > +  if (dev->type != &ufshpb_dev_type)
> > +    return -ENODEV;
> > +
> > +  ufsf = container_of(dev, struct ufsf_feature_info, hpb_dev);
> > +  hba = container_of(ufsf, struct ufs_hba, ufsf);
> > +
> > +  async_schedule(ufshpb_probe_async, hba);
> > +  return 0;
> > +}

> So this is an asynchronous probe that is not visible to the device
> driver core? Could the PROBE_PREFER_ASYNCHRONOUS flag have been used
> instead to make device probing asynchronous?
I added the PROBE_PREFER_ASYNCHRONOUS flag to code and changed it to 
probe synchronously.
 
> > +static int ufshpb_remove(struct device *dev)
> > +{
> > +  struct ufshpb_lu *hpb, *n_hpb;
> > +  struct ufsf_feature_info *ufsf;
> > +  struct scsi_device *sdev;
> > +
> > +  ufsf = container_of(dev, struct ufsf_feature_info, hpb_dev);
> > +
> > +  dev_set_drvdata(&ufsf->hpb_dev, NULL);
> > +
> > +  list_for_each_entry_safe(hpb, n_hpb, &ufshpb_drv.lh_hpb_lu,
> > +         list_hpb_lu) {
> > +    ufshpb_set_state(hpb, HPB_FAILED);
> > +
> > +    sdev = hpb->sdev_ufs_lu;
> > +    sdev->hostdata = NULL;
> > +
> > +    device_del(&hpb->hpb_lu_dev);
> > +
> > +    dev_info(&hpb->hpb_lu_dev, "hpb_lu_dev refcnt %d\n",
> > +       kref_read(&hpb->hpb_lu_dev.kobj.kref));
> > +    put_device(&hpb->hpb_lu_dev);
> > +  }
> > +  dev_info(dev, "ufshpb: remove success\n");
> > +
> > +  return 0;
> > +}

> Where is the code that waits for the asynchronously scheduled probe
> calls to finish?
I changed it to probe without async_schedule.

> > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
> > new file mode 100644
> > index 000000000000..c6dd88e00849
> > --- /dev/null
> > +++ b/drivers/scsi/ufs/ufshpb.h
> > @@ -0,0 +1,185 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Universal Flash Storage Host Performance Booster
> > + *
> > + * Copyright (C) 2017-2018 Samsung Electronics Co., Ltd.
> > + *
> > + * Authors:
> > + *  Yongmyung Lee <ymhungry.lee@xxxxxxxxxxx>
> > + *  Jinyoung Choi <j-young.choi@xxxxxxxxxxx>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + * See the COPYING file in the top-level directory or visit
> > + * <http://www.gnu.org/licenses/gpl-2.0.html>
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * This program is provided "AS IS" and "WITH ALL FAULTS" and
> > + * without warranty of any kind. You are solely responsible for
> > + * determining the appropriateness of using and distributing
> > + * the program and assume all risks associated with your exercise
> > + * of rights with respect to the program, including but not limited
> > + * to infringement of third party rights, the risks and costs of
> > + * program errors, damage to or loss of data, programs or equipment,
> > + * and unavailability or interruption of operations. Under no
> > + * circumstances will the contributor of this Program be liable for
> > + * any damages of any kind arising from your use or distribution of
> > + * this program.
> > + *
> > + * The Linux Foundation chooses to take subject only to the GPLv2
> > + * license terms, and distributes only under these terms.
> > + */

> Please use an SPDX declaration instead of the full GPLv2 text.
OK, I will.

Thanks,
Daejun.