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  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.

Powered by blists - more mailing lists