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: <f76e4427-bb7b-747a-fd5a-21ca9e388d4a@amd.com>
Date: Mon, 12 Aug 2024 08:58:04 -0700
From: Lizhi Hou <lizhi.hou@....com>
To: Carl Vanderlip <quic_carlv@...cinc.com>, <ogabbay@...nel.org>,
	<dri-devel@...ts.freedesktop.org>
CC: <linux-kernel@...r.kernel.org>, <min.ma@....com>, <max.zhen@....com>,
	<sonal.santan@....com>, <king.tam@....com>, Narendra Gutta
	<VenkataNarendraKumar.Gutta@....com>, George Yang <George.Yang@....com>
Subject: Re: [PATCH V2 01/10] accel/amdxdna: Add a new driver for AMD AI
 Engine


On 8/9/24 08:24, Carl Vanderlip wrote:
> On 8/5/2024 10:39 AM, Lizhi Hou wrote:
> > +static int aie2_init(struct amdxdna_dev *xdna)
> > +{
> > +    struct pci_dev *pdev = to_pci_dev(xdna->ddev.dev);
> > +    struct amdxdna_dev_hdl *ndev;
> > +    struct psp_config psp_conf;
> > +    const struct firmware *fw;
> > +    void __iomem * const *tbl;
> > +    int i, bars, nvec, ret;
> > +
> > +    ndev = devm_kzalloc(&pdev->dev, sizeof(*ndev), GFP_KERNEL);
> > +    if (!ndev)
> > +        return -ENOMEM;
> > +
> > +    ndev->priv = xdna->dev_info->dev_priv;
> > +    ndev->xdna = xdna;
> > +
> > +    ret = request_firmware(&fw, ndev->priv->fw_path, &pdev->dev);
> > +    if (ret) {
> > +        XDNA_ERR(xdna, "failed to request_firmware %s, ret %d",
> > +             ndev->priv->fw_path, ret);
> > +        return ret;
> > +    }
> > +
> > +    ret = pcim_enable_device(pdev);
>
>
> Does request_firmware need to be the first thing here? Its not used 
> until the end of init. Likewise, fw image is copied in *_create, but 
> then not released until after *_hw_start; could release_firmware more 
> closely wrap where it is used? I could see it being checked first 
> because if the fw isn't there, what's the point, but that could be 
> said about any of the other resources here.
request_firmware() will failed if user forget to install the firmware 
package. Other initialization calls (e.g. enable device, etc) are very 
unlikely to happen.  That is why request_firmware() is checked first. 
This will only hold the memory before the function exits. I think it is 
very short period and should be ok.
>
> On 8/5/2024 10:39 AM, Lizhi Hou wrote:
> > +enum aie2_smu_reg_idx {
> > +    SMU_CMD_REG = 0,
> > +    SMU_ARG_REG,
> > +    SMU_INTR_REG,
> > +    SMU_RESP_REG,
> > +    SMU_OUT_REG,
> > +    SMU_MAX_REGS /* Kepp this at the end */
> > +};
>
> *Keep

Thanks. I will fix this.


Lizhi

>
>
> -Carl V.
>
> PS Sorry for double email Lizhi, forgot to send to list.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ