netdev - Re: [PATCH v5 08/16] IB/pvrdma: Add device command support

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]

Message-ID: <3697a394-4bb0-b745-1ae2-5e0583a53a96@vmware.com>
Date:   Mon, 26 Sep 2016 11:06:06 -0700
From:   Adit Ranadive <aditr@...are.com>
To:     Yuval Shaia <yuval.shaia@...cle.com>
CC:     <dledford@...hat.com>, <linux-rdma@...r.kernel.org>,
        <pv-drivers@...are.com>, <netdev@...r.kernel.org>,
        <linux-pci@...r.kernel.org>, <jhansen@...are.com>,
        <asarwade@...are.com>, <georgezhang@...are.com>,
        <bryantan@...are.com>
Subject: Re: [PATCH v5 08/16] IB/pvrdma: Add device command support

On Mon, Sep 26, 2016 at 00:16:01AM -0700, Yuval Shaia wrote:
> Minor question/suggestion inline.
> (sorry for missing it till now).
> 
> Yuval

<...>

> > +int
> > +pvrdma_cmd_post(struct pvrdma_dev *dev, union pvrdma_cmd_req *req,
> > +		union pvrdma_cmd_resp *resp, unsigned resp_code)
> > +{
> > +	int err;
> > +
> > +	dev_dbg(&dev->pdev->dev, "post request to device\n");
> > +
> > +	/* Serializiation */
> > +	down(&dev->cmd_sema);
> > +
> > +	BUILD_BUG_ON(sizeof(union pvrdma_cmd_req) !=
> > +		     sizeof(struct pvrdma_cmd_modify_qp));
> > +
> > +	spin_lock(&dev->cmd_lock);
> > +	memcpy(dev->cmd_slot, req, sizeof(*req));
> > +	spin_unlock(&dev->cmd_lock);
> > +
> > +	init_completion(&dev->cmd_done);
> > +	pvrdma_write_reg(dev, PVRDMA_REG_REQUEST, 0);
> > +
> > +	/* Make sure the request is written before reading status. */
> > +	mb();
> > +
> > +	err = pvrdma_read_reg(dev, PVRDMA_REG_ERR);
> > +	if (err == 0) {
> > +		if (resp != NULL)
> > +			err = pvrdma_cmd_recv(dev, resp, resp_code);
> > +	} else {
> > +		dev_warn(&dev->pdev->dev, "failed to write request %d\n", err);
> > +	}
> 
> Please note that callers checks if err<0  while here err!=0 considered as
> error.
> Looking at similar code in pvrdma_pci_probe i can see that function
> "translates" this error to -EINVAL. Suggesting to do the same here
> (although -EINVAL is not a good choice, if device can be more friendly with
> the return code then we can propagate it up or just print a suitable error
> i.e. EAGAIN or ENOMEM otherwise EFAULT should be sufficient).
>

Thanks for pointing this out.
I dont think we return a positive error from the error register. So the
(err == 0) should be (err >= 0). Also, I'll set the err to -EFAULT if
read_reg fails (based on your feedback on the other patch).