RE: [PATCH V3 1/2] crypto: Add Imagination Technologies hw hash accelerator

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Andrew, 

> -----Original Message-----
> From: abrestic@xxxxxxxxxx [mailto:abrestic@xxxxxxxxxx] On Behalf Of
> Andrew Bresticker
> Sent: 10 March 2015 18:02
> To: James Hartley
> Cc: linux-crypto@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH V3 1/2] crypto: Add Imagination Technologies hw hash
> accelerator
> 
> Hi James,
> 
> >> > +static irqreturn_t img_irq_handler(int irq, void *dev_id) {
> >> > +       struct img_hash_dev *hdev = dev_id;
> >> > +       u32 reg;
> >> > +
> >> > +       reg = img_hash_read(hdev, CR_INTSTAT);
> >> > +       img_hash_write(hdev, CR_INTCLEAR, reg);
> >> > +
> >> > +       if (reg & CR_INT_NEW_RESULTS_SET) {
> >> > +               dev_dbg(hdev->dev, "IRQ CR_INT_NEW_RESULTS_SET\n");
> >> > +               if (DRIVER_FLAGS_BUSY & hdev->flags) {
> >> > +                       hdev->flags |= DRIVER_FLAGS_OUTPUT_READY;
> >> > +                       if (!(DRIVER_FLAGS_CPU & hdev->flags))
> >> > +                               hdev->flags |= DRIVER_FLAGS_DMA_READY;
> >> > +                       tasklet_schedule(&hdev->done_task);
> >>
> >> Since this done_task only gets scheduled from here, why not make this
> >> a threaded IRQ handler and just do the work here instead of
> >> separating it between a hard IRQ handler and a tasklet?
> >
> > What is the advantage of doing that?  i.e is this simple case worth setting up
> an extra thread?
> 
> I believe threaded IRQ handlers are now preferred to using a hard IRQ
> handler + tasklet when possible, partly because people tend to screw up
> tasklet usage.

Fair enough, but I'd like to leave this as an enhancement for when I have some
extra bandwidth if it's not essential (same for the runtime PM).  

> 
> >> > +               err = PTR_ERR(hdev->io_base);
> >> > +               goto hash_io_err;
> >> > +       }
> >> > +
> >> > +       /* Write port (DMA or CPU) */
> >> > +       hash_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> >> > +       if (!hash_res) {
> >> > +               dev_err(dev, "no write port resource info\n");
> >> > +               err = -ENODEV;
> >> > +               goto res_err;
> >> > +       }
> >> > +       hdev->bus_addr = hash_res->start;
> >>
> >> Maybe set this after devm_ioremap_resource() succeeds and avoid the
> >> extra error check?
> >
> > Not quite following you here - which check are you saying can be removed?
> 
> You can remove the if (hash_res) check if you set hdev->bus_addr after
> devm_ioremap_resource().

I see what you mean - done.

> 
> Thanks,
> Andrew

Thanks, 
James.
��.n��������+%������w��{.n�����{���{ay�ʇڙ���f���h������_�(�階�ݢj"��������G����?���&��





[Index of Archives]     [Kernel]     [Gnu Classpath]     [Gnu Crypto]     [DM Crypt]     [Netfilter]     [Bugtraq]

  Powered by Linux