On 2020-06-04 18:38, Daejun Park wrote: > + 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. > +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. > +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. > +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? > +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? > +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? > 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. Thanks, Bart.