| 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
| ||
|
Message-ID: <CALMYJDuXWT4=JEC=PrC9j+csb0mL8-P7zrZV1C4qdkZLg0HPEw@mail.gmail.com>
Date: Tue, 7 Feb 2012 20:40:10 +0530
From: Santosh Y <santoshsy@...il.com>
To: balbi@...com
Cc: Vinayak Holikatti <vinholikatti@...il.com>,
James.Bottomley@...senpartnership.com, linux-scsi@...r.kernel.org,
linux-kernel@...r.kernel.org, patches@...aro.org,
linux-samsung-soc@...r.kernel.org, saugata.das@...aro.org,
arnd@...db.de, venkat@...aro.org, girish.shivananjappa@...aro.org,
vishak.g@...sung.com, k.rajesh@...sung.com, yejin.moon@...sung.com
Subject: Re: [PATCH 1/4] [SCSI] ufshcd: UFS Host controller driver
On Tue, Feb 7, 2012 at 12:46 PM, Felipe Balbi <balbi@...com> wrote:
> Hi,
>
> On Thu, Feb 02, 2012 at 10:27:26AM +0530, Vinayak Holikatti wrote:
>> +/**
>> + * ufshcd_uic_cc_handler - handle UIC command completion
>> + * @work: pointer to a work queue structure
>> + *
>> + * Returns 0 on success, non-zero value on failure
>> + */
>> +static void ufshcd_uic_cc_handler (struct work_struct *work)
>> +{
>> + struct ufs_hba *hba;
>> +
>> + hba = container_of(work, struct ufs_hba, uic_workq);
>> +
>> + if ((UIC_CMD_DME_LINK_STARTUP == hba->active_uic_cmd.command) &&
>
> this is so annoying. Please invert all these constructs:
>
> if ((hcd->active_uic_cmd.command == UIC_CMD_DME_LINK_STARTUP) ....
> and so on.
>
ok, I'll change it.
>> + !(ufshcd_get_uic_cmd_result(hba))) {
>> +
>> + if (ufshcd_make_hba_operational(hba))
>> + dev_err(&hba->pdev->dev,
>> + "cc: hba not operational state\n");
>> + return;
>> + }
>> +}
>> +
>> +/**
>> + * ufshcd_sl_intr - Interrupt service routine
>> + * @hba: per adapter instance
>> + * @intr_status: contains interrupts generated by the controller
>> + */
>> +static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
>> +{
>> + if (intr_status & UIC_COMMAND_COMPL)
>> + schedule_work(&hba->uic_workq);
>> +}
>> +
>> +/**
>> + * ufshcd_intr - Main interrupt service routine
>> + * @irq: irq number
>> + * @__hba: pointer to adapter instance
>> + *
>> + * Returns IRQ_HANDLED - If interrupt is valid
>> + * IRQ_NONE - If invalid interrupt
>> + */
>> +static irqreturn_t ufshcd_intr(int irq, void *__hba)
>> +{
>> + unsigned long flags;
>> + u32 intr_status;
>> + irqreturn_t retval = IRQ_NONE;
>> + struct ufs_hba *hba = __hba;
>> +
>> + spin_lock_irqsave(hba->host->host_lock, flags);
>> + intr_status = readl(UFSHCD_MMIO_BASE + REG_INTERRUPT_STATUS);
>> +
>> + if (intr_status) {
>> + ufshcd_sl_intr(hba, intr_status);
>> +
>> + /* If UFSHCI 1.0 then clear interrupt status register */
>> + if (UFSHCI_VERSION_10 == hba->ufs_version)
>> + writel(intr_status,
>> + (UFSHCD_MMIO_BASE + REG_INTERRUPT_STATUS));
>> + retval = IRQ_HANDLED;
>> + }
>> + spin_unlock_irqrestore(hba->host->host_lock, flags);
>> + return retval;
>> +}
>> +
>> +static struct scsi_host_template ufshcd_driver_template = {
>> + .module = THIS_MODULE,
>> + .name = UFSHCD,
>> + .proc_name = UFSHCD,
>> + .this_id = -1,
>> +};
>> +
>> +/**
>> + * ufshcd_shutdown - main funciton to put the controller in reset state
>> + * @pdev: pointer to PCI device handle
>> + */
>> +static void ufshcd_shutdown(struct pci_dev *pdev)
>> +{
>> + ufshcd_hba_stop((struct ufs_hba *)pci_get_drvdata(pdev));
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +/**
>> + * ufshcd_suspend - suspend power management function
>> + * @pdev: pointer to PCI device handle
>> + * @state: power state
>> + *
>> + * Returns -ENOSYS
>> + */
>> +static int ufshcd_suspend(struct pci_dev *pdev, pm_message_t state)
>> +{
>> + return -ENOSYS;
>> +}
>> +
>> +/**
>> + * ufshcd_resume - resume power management function
>> + * @pdev: pointer to PCI device handle
>> + *
>> + * Returns -ENOSYS
>> + */
>> +static int ufshcd_resume(struct pci_dev *pdev)
>> +{
>> + return -ENOSYS;
>> +}
>> +#endif /* CONFIG_PM */
>> +
>> +/**
>> + * ufshcd_hba_free - free allocated memory for
>> + * host memory space data structures
>> + * @hba: per adapter instance
>> + */
>> +static void ufshcd_hba_free(struct ufs_hba *hba)
>> +{
>> + iounmap(UFSHCD_MMIO_BASE);
>> + ufshcd_free_hba_memory(hba);
>> + pci_release_regions(hba->pdev);
>> +}
>> +
>> +/**
>> + * ufshcd_remove - deallocate PCI/SCSI host and host memory space
>> + * data structure memory
>> + * @pdev - pointer to PCI handle
>> + */
>> +static void ufshcd_remove(struct pci_dev *pdev)
>> +{
>> + struct ufs_hba *hba = pci_get_drvdata(pdev);
>> +
>> + /* disable interrupts */
>> + ufshcd_int_config(hba, UFSHCD_INT_DISABLE);
>> + free_irq(pdev->irq, hba);
>> +
>> + ufshcd_hba_stop(hba);
>> + ufshcd_hba_free(hba);
>> +
>> + scsi_remove_host(hba->host);
>> + scsi_host_put(hba->host);
>> + pci_set_drvdata(pdev, NULL);
>> + pci_clear_master(pdev);
>> + pci_disable_device(pdev);
>> +}
>> +
>> +/**
>> + * ufshcd_set_dma_mask - Set dma addressing
>> + * @pdev: PCI device struct
>> + *
>> + * Returns 0 for success, non-zero for failure
>> + */
>> +static int ufshcd_set_dma_mask(struct pci_dev *pdev)
>> +{
>> + int err;
>> +
>> + do {
>> + err = pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
>> + if (!err) {
>> + err = pci_set_consistent_dma_mask(pdev,
>> + DMA_BIT_MASK(64));
>> + break;
>> + }
>> + err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
>> + if (!err)
>> + err = pci_set_consistent_dma_mask(pdev,
>> + DMA_BIT_MASK(32));
>> + } while (0);
>> +
>> + return err;
>> +}
>> +
>> +/**
>> + * ufshcd_probe - probe routine of the driver
>> + * @pdev: pointer to PCI device handle
>> + * @id: PCI device id
>> + *
>> + * Returns 0 on success, non-zero value on failure
>> + */
>> +static int __devinit
>> +ufshcd_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> +{
>> + struct Scsi_Host *host;
>> + struct ufs_hba *hba;
>> + int ufs_hba_len;
>> + int err;
>> +
>> + ufs_hba_len = sizeof(struct ufs_hba);
>> + err = pci_enable_device(pdev);
>> + if (err) {
>> + dev_err(&pdev->dev, "pci_enable_device failed\n");
>> + goto out_error;
>> + }
>> +
>> + pci_set_master(pdev);
>> +
>> + host = scsi_host_alloc(&ufshcd_driver_template, ufs_hba_len);
>> + if (!host) {
>> + dev_err(&pdev->dev, "scsi_host_alloc failed\n");
>> + err = -ENOMEM;
>> + goto out_disable;
>> + }
>> + hba = (struct ufs_hba *)host->hostdata;
>> +
>> + err = pci_request_regions(pdev, UFSHCD);
>> + if (err < 0) {
>> + dev_err(&pdev->dev, "request regions failed\n");
>> + goto out_disable;
>> + }
>> +
>> + hba->mmio_base = pci_ioremap_bar(pdev, 0);
>> + if (!hba->mmio_base) {
>> + dev_err(&pdev->dev, "memory map failed\n");
>> + err = -ENOMEM;
>> + goto out_release_regions;
>> + }
>> +
>> + err = ufshcd_set_dma_mask(pdev);
>> + if (err) {
>> + dev_err(&pdev->dev, "set dma mask failed\n");
>> + goto out_iounmap;
>> + }
>> +
>> + hba->host = host;
>> + hba->pdev = pdev;
>> +
>> + /* Read capabilities registers */
>> + ufshcd_hba_capabilities(hba);
>> +
>> + /* Get UFS version supported by the controller */
>> + hba->ufs_version = ufshcd_get_ufs_version(hba);
>> +
>> + /* Allocate memory for host memory space */
>> + err = ufshcd_memory_alloc(hba);
>> + if (err) {
>> + dev_err(&pdev->dev, "Memory allocation failed\n");
>> + goto out_iounmap;
>> + }
>> +
>> + /* Configure LRB */
>> + ufshcd_host_memory_configure(hba);
>> +
>> + host->can_queue = hba->nutrs;
>> + host->max_id = UFSHCD_MAX_ID;
>> + host->max_lun = UFSHCD_MAX_LUNS;
>> + host->max_channel = UFSHCD_MAX_CHANNEL;
>> + host->unique_id = host->host_no;
>> +
>> + /* Initialize work queues */
>> + INIT_WORK(&hba->uic_workq, ufshcd_uic_cc_handler);
>> +
>> + /* IRQ registration */
>> + err = request_irq(pdev->irq, ufshcd_intr, IRQF_SHARED, UFSHCD, hba);
>
> make this a threaded IRQ and remove your workqueues and tasklets.
>
ok, I'll look into it.
>> + if (err) {
>> + dev_err(&pdev->dev, "request irq failed\n");
>> + goto out_lrb_free;
>> + }
>> +
>> + pci_set_drvdata(pdev, hba);
>
> would also be cool if you would abstract PCI out of this driver, so it's
> easier to be re-used. See how I did it with drivers/usb/dwc3/core.c
> drivers/usb/dwc3/dwc3-pci.c and drivers/usb/dwc3/dwc3-omap.c
>
ok, we'll send it as a separate patch.
> --
> balbi
Thanks,
~Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists