linux-kernel - Re: [RFC PATCH 09/13] scsi: ufshpb: Add response API

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 15 May 2020 20:06:28 -0700
From:   Bart Van Assche <bvanassche@....org>
To:     Avri Altman <avri.altman@....com>,
        "James E . J . Bottomley" <jejb@...ux.vnet.ibm.com>,
        "Martin K . Petersen" <martin.petersen@...cle.com>,
        linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     alim.akhtar@...sung.com, asutoshd@...eaurora.org,
        Zang Leigang <zangleigang@...ilicon.com>,
        Avi Shchislowski <avi.shchislowski@....com>,
        Bean Huo <beanhuo@...ron.com>, cang@...eaurora.org,
        stanley.chu@...iatek.com,
        MOHAMMED RAFIQ KAMAL BASHA <md.rafiq@...sung.com>,
        Sang-yoon Oh <sangyoon.oh@...sung.com>,
        yongmyung lee <ymhungry.lee@...sung.com>,
        Jinyoung CHOI <j-young.choi@...sung.com>
Subject: Re: [RFC PATCH 09/13] scsi: ufshpb: Add response API

On 2020-05-15 03:30, Avri Altman wrote:
> +#define RSP_DATA_SEG_LEN (sizeof(struct ufshpb_sense_data))

The name of this macro is almost as long as the expression it replaces.
It may make the code easier to read by dropping this macro and using the
sizeof() expression directly.

> +	struct tasklet_struct rsp_tasklet;

Why a tasklet instead of e.g. a work_struct? Tasklets can cause nasty
problems, e.g. CPU lockup complaints if too much work is done in tasklet
context.

> +static void ufshpb_dh_notify(struct ufshpb_lun *hpb,
> +			     struct ufshpb_sense_data *sense)
> +{
> +	struct ufs_hba *hba = shost_priv(hpb->sdev->host);
> +
> +	spin_lock(hba->host->host_lock);
> +
> +	if (scsi_device_get(hpb->sdev)) {
> +		spin_unlock(hba->host->host_lock);
> +		return;
> +	}
> +
> +	scsi_dh_set_params(hpb->sdev->request_queue, (const char *)sense);
> +
> +	scsi_device_put(hpb->sdev);
> +
> +	spin_unlock(hba->host->host_lock);
> +}

To me this looks like slight abuse of the scsi_dh_set_params() function.
The documentation of that function mentions clearly that the second
argument is an ASCII string and not e.g. sense data.

Has this driver been tested on a system with lockdep enabled? I don't
think that it is acceptable to use spin_lock() in tasklet context.

> +static void ufshpb_tasklet_fn(unsigned long priv)
> +{
> +	struct ufshpb_lun *hpb = (struct ufshpb_lun *)priv;
> +	struct ufshpb_rsp_element *rsp_elem = NULL;
> +	unsigned long flags;
> +
> +	while (1) {
> +		spin_lock_irqsave(&hpb->rsp_list_lock, flags);
> +		rsp_elem = ufshpb_get_rsp_elem(hpb, &hpb->lh_rsp);
> +		spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
> +
> +		if (!rsp_elem)
> +			return;
> +
> +		ufshpb_dh_notify(hpb, &rsp_elem->sense_data);
> +
> +		spin_lock_irqsave(&hpb->rsp_list_lock, flags);
> +		list_add_tail(&rsp_elem->list, &hpb->lh_rsp_free);
> +		spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
> +	}
> +}

Please schedule work instead of using tasklet context.

Thanks,

Bart.