Message ID | 1468454751-12466-5-git-send-email-hch@lst.de |
---|---|
State | Not Applicable |
Delegated to: | David Miller |
Headers | show |
Hello, I'm still a bit confused about this patch. On Thu, Jul 14, 2016 at 09:05:45AM +0900, Christoph Hellwig wrote: > diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c > index a723ae9..acfb865 100644 > --- a/drivers/ata/sata_fsl.c > +++ b/drivers/ata/sata_fsl.c > @@ -534,7 +534,7 @@ static void sata_fsl_qc_prep(struct ata_queued_cmd *qc) > VPRINTK("Dumping cfis : 0x%x, 0x%x, 0x%x\n", > cd->cfis[0], cd->cfis[1], cd->cfis[2]); > > - if (qc->tf.protocol == ATA_PROT_NCQ) { > + if (ata_is_fpdma(qc->tf.protocol)) { ata_is_fpdma() tests NCQ or DMA. Is this right? > --- a/drivers/ata/sata_mv.c > +++ b/drivers/ata/sata_mv.c > @@ -1173,7 +1173,7 @@ static void mv_set_irq_coalescing(struct ata_host *host, > static void mv_start_edma(struct ata_port *ap, void __iomem *port_mmio, > struct mv_port_priv *pp, u8 protocol) > { > - int want_ncq = (protocol == ATA_PROT_NCQ); > + int want_ncq = (ata_is_fpdma(protocol)); Let's get rid of the parentheses while at it. > > if (pp->pp_flags & MV_PP_FLAG_EDMA_EN) { > int using_ncq = ((pp->pp_flags & MV_PP_FLAG_NCQ_EN) != 0); > @@ -2156,8 +2156,7 @@ static void mv_qc_prep_iie(struct ata_queued_cmd *qc) > unsigned in_index; > u32 flags = 0; > > - if ((tf->protocol != ATA_PROT_DMA) && > - (tf->protocol != ATA_PROT_NCQ)) > + if (!ata_is_fpdma(tf->protocol)) This is actually testing !DMA && !NCQ and an equivalent conversion but the rest aren't. > diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c > index 734f563..89e36aa 100644 > --- a/drivers/ata/sata_nv.c > +++ b/drivers/ata/sata_nv.c > @@ -1407,7 +1407,7 @@ static void nv_adma_qc_prep(struct ata_queued_cmd *qc) > cpb->next_cpb_idx = 0; > > /* turn on NCQ flags for NCQ commands */ > - if (qc->tf.protocol == ATA_PROT_NCQ) > + if (ata_is_fpdma(qc->tf.protocol)) > ctl_flags |= NV_CPB_CTL_QUEUE | NV_CPB_CTL_FPDMA; And I don't know why testing NCQ or DMA would be safe for places like this. > + ATA_PROT_FLAG_FPDMA = ATA_PROT_FLAG_NCQ | ATA_PROT_FLAG_DMA, ... > +static inline bool ata_is_fpdma(u8 prot) > +{ > + return ata_prot_flags(prot) & ATA_PROT_FLAG_FPDMA; > +} ??? Thanks.
On 07/14/2016 04:37 PM, Tejun Heo wrote: > Hello, > > I'm still a bit confused about this patch. > > On Thu, Jul 14, 2016 at 09:05:45AM +0900, Christoph Hellwig wrote: >> diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c >> index a723ae9..acfb865 100644 >> --- a/drivers/ata/sata_fsl.c >> +++ b/drivers/ata/sata_fsl.c >> @@ -534,7 +534,7 @@ static void sata_fsl_qc_prep(struct ata_queued_cmd *qc) >> VPRINTK("Dumping cfis : 0x%x, 0x%x, 0x%x\n", >> cd->cfis[0], cd->cfis[1], cd->cfis[2]); >> >> - if (qc->tf.protocol == ATA_PROT_NCQ) { >> + if (ata_is_fpdma(qc->tf.protocol)) { > > ata_is_fpdma() tests NCQ or DMA. Is this right? > Yes. The 'sata_fsl' driver has no means (or at least none which I could detect) allowing it to send NCQ NON-DATA commands. So for this driver the check for ncq is actually a check for fpdma. >> --- a/drivers/ata/sata_mv.c >> +++ b/drivers/ata/sata_mv.c >> @@ -1173,7 +1173,7 @@ static void mv_set_irq_coalescing(struct ata_host *host, >> static void mv_start_edma(struct ata_port *ap, void __iomem *port_mmio, >> struct mv_port_priv *pp, u8 protocol) >> { >> - int want_ncq = (protocol == ATA_PROT_NCQ); >> + int want_ncq = (ata_is_fpdma(protocol)); > > Let's get rid of the parentheses while at it. > Okay. >> >> if (pp->pp_flags & MV_PP_FLAG_EDMA_EN) { >> int using_ncq = ((pp->pp_flags & MV_PP_FLAG_NCQ_EN) != 0); >> @@ -2156,8 +2156,7 @@ static void mv_qc_prep_iie(struct ata_queued_cmd *qc) >> unsigned in_index; >> u32 flags = 0; >> >> - if ((tf->protocol != ATA_PROT_DMA) && >> - (tf->protocol != ATA_PROT_NCQ)) >> + if (!ata_is_fpdma(tf->protocol)) > > This is actually testing !DMA && !NCQ and an equivalent conversion but > the rest aren't. > Yes. >> diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c >> index 734f563..89e36aa 100644 >> --- a/drivers/ata/sata_nv.c >> +++ b/drivers/ata/sata_nv.c >> @@ -1407,7 +1407,7 @@ static void nv_adma_qc_prep(struct ata_queued_cmd *qc) >> cpb->next_cpb_idx = 0; >> >> /* turn on NCQ flags for NCQ commands */ >> - if (qc->tf.protocol == ATA_PROT_NCQ) >> + if (ata_is_fpdma(qc->tf.protocol)) >> ctl_flags |= NV_CPB_CTL_QUEUE | NV_CPB_CTL_FPDMA; > > And I don't know why testing NCQ or DMA would be safe for places like > this. > Again, this driver cannot handle NCQ NON-DATA command, so it always has to assume that NCQ _actually_ means FPDMA. >> + ATA_PROT_FLAG_FPDMA = ATA_PROT_FLAG_NCQ | ATA_PROT_FLAG_DMA, > ... >> +static inline bool ata_is_fpdma(u8 prot) >> +{ >> + return ata_prot_flags(prot) & ATA_PROT_FLAG_FPDMA; >> +} > > ??? > Yeah, strictly speaking there is no need for ATA_PROT_FLAG_FPDMA. I'll be removing it if you insist. Cheers, Hannes -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Hannes. On Thu, Jul 14, 2016 at 05:15:51PM +0200, Hannes Reinecke wrote: > >> +static inline bool ata_is_fpdma(u8 prot) > >> +{ > >> + return ata_prot_flags(prot) & ATA_PROT_FLAG_FPDMA; > >> +} > > > > ??? > > > Yeah, strictly speaking there is no need for ATA_PROT_FLAG_FPDMA. > I'll be removing it if you insist. No, it's just really confusing to have a function named ata_is_fpdma() and then have it test whether the protocol is NCQ or DMA. If you wanna do that, name it ata_is_dma_or_fpdma(). ata_is_fpdma() should test (prot & ATA_PROT_FLAG_FPDMA) == ATA_PROT_FLAG_FPDMA. Thanks.
On 07/14/2016 05:19 PM, Tejun Heo wrote: > Hello, Hannes. > > On Thu, Jul 14, 2016 at 05:15:51PM +0200, Hannes Reinecke wrote: >>>> +static inline bool ata_is_fpdma(u8 prot) >>>> +{ >>>> + return ata_prot_flags(prot) & ATA_PROT_FLAG_FPDMA; >>>> +} >>> >>> ??? >>> >> Yeah, strictly speaking there is no need for ATA_PROT_FLAG_FPDMA. >> I'll be removing it if you insist. > > No, it's just really confusing to have a function named ata_is_fpdma() > and then have it test whether the protocol is NCQ or DMA. If you > wanna do that, name it ata_is_dma_or_fpdma(). ata_is_fpdma() should > test (prot & ATA_PROT_FLAG_FPDMA) == ATA_PROT_FLAG_FPDMA. > Ah. Now I see your point. That was unintentional, and your suggestion is in fact correct. I'll be fixing it up. Cheers, Hannes
Hi Rejun, hi Hannes, Damien and I spent some time going over this patch today and we think it's best to simply drop it. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/15/2016 08:13 AM, Christoph Hellwig wrote: > Hi Rejun, hi Hannes, > > Damien and I spent some time going over this patch today and we think > it's best to simply drop it. > Sure, fine with me. Cheers, Hannes
On Fri, Jul 15, 2016 at 09:07:30AM +0200, Hannes Reinecke wrote: > > Damien and I spent some time going over this patch today and we think > > it's best to simply drop it. > > > Sure, fine with me. Great. I've verified that the remaining patches will apply and work fine when simply dropping it. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c index a723ae9..acfb865 100644 --- a/drivers/ata/sata_fsl.c +++ b/drivers/ata/sata_fsl.c @@ -534,7 +534,7 @@ static void sata_fsl_qc_prep(struct ata_queued_cmd *qc) VPRINTK("Dumping cfis : 0x%x, 0x%x, 0x%x\n", cd->cfis[0], cd->cfis[1], cd->cfis[2]); - if (qc->tf.protocol == ATA_PROT_NCQ) { + if (ata_is_fpdma(qc->tf.protocol)) { VPRINTK("FPDMA xfer,Sctor cnt[0:7],[8:15] = %d,%d\n", cd->cfis[3], cd->cfis[11]); } @@ -551,7 +551,7 @@ static void sata_fsl_qc_prep(struct ata_queued_cmd *qc) &ttl_dwords, cd_paddr, host_priv->data_snoop); - if (qc->tf.protocol == ATA_PROT_NCQ) + if (ata_is_fpdma(qc->tf.protocol)) desc_info |= FPDMA_QUEUED_CMD; sata_fsl_setup_cmd_hdr_entry(pp, tag, desc_info, ttl_dwords, diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c index bd74ee5..a81b1f8 100644 --- a/drivers/ata/sata_mv.c +++ b/drivers/ata/sata_mv.c @@ -1173,7 +1173,7 @@ static void mv_set_irq_coalescing(struct ata_host *host, static void mv_start_edma(struct ata_port *ap, void __iomem *port_mmio, struct mv_port_priv *pp, u8 protocol) { - int want_ncq = (protocol == ATA_PROT_NCQ); + int want_ncq = (ata_is_fpdma(protocol)); if (pp->pp_flags & MV_PP_FLAG_EDMA_EN) { int using_ncq = ((pp->pp_flags & MV_PP_FLAG_NCQ_EN) != 0); @@ -2156,8 +2156,7 @@ static void mv_qc_prep_iie(struct ata_queued_cmd *qc) unsigned in_index; u32 flags = 0; - if ((tf->protocol != ATA_PROT_DMA) && - (tf->protocol != ATA_PROT_NCQ)) + if (!ata_is_fpdma(tf->protocol)) return; if (tf->command == ATA_CMD_DSM) return; /* use bmdma for this */ diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c index 734f563..89e36aa 100644 --- a/drivers/ata/sata_nv.c +++ b/drivers/ata/sata_nv.c @@ -1407,7 +1407,7 @@ static void nv_adma_qc_prep(struct ata_queued_cmd *qc) cpb->next_cpb_idx = 0; /* turn on NCQ flags for NCQ commands */ - if (qc->tf.protocol == ATA_PROT_NCQ) + if (ata_is_fpdma(qc->tf.protocol)) ctl_flags |= NV_CPB_CTL_QUEUE | NV_CPB_CTL_FPDMA; VPRINTK("qc->flags = 0x%lx\n", qc->flags); @@ -1432,15 +1432,14 @@ static unsigned int nv_adma_qc_issue(struct ata_queued_cmd *qc) { struct nv_adma_port_priv *pp = qc->ap->private_data; void __iomem *mmio = pp->ctl_block; - int curr_ncq = (qc->tf.protocol == ATA_PROT_NCQ); + int curr_ncq = ata_is_fpdma(qc->tf.protocol); VPRINTK("ENTER\n"); /* We can't handle result taskfile with NCQ commands, since retrieving the taskfile switches us out of ADMA mode and would abort existing commands. */ - if (unlikely(qc->tf.protocol == ATA_PROT_NCQ && - (qc->flags & ATA_QCFLAG_RESULT_TF))) { + if (unlikely(curr_ncq && (qc->flags & ATA_QCFLAG_RESULT_TF))) { ata_dev_err(qc->dev, "NCQ w/ RESULT_TF not allowed\n"); return AC_ERR_SYSTEM; } @@ -1991,7 +1990,7 @@ static int nv_swncq_port_start(struct ata_port *ap) static void nv_swncq_qc_prep(struct ata_queued_cmd *qc) { - if (qc->tf.protocol != ATA_PROT_NCQ) { + if (!ata_is_fpdma(qc->tf.protocol)) { ata_bmdma_qc_prep(qc); return; } @@ -2067,7 +2066,7 @@ static unsigned int nv_swncq_qc_issue(struct ata_queued_cmd *qc) struct ata_port *ap = qc->ap; struct nv_swncq_port_priv *pp = ap->private_data; - if (qc->tf.protocol != ATA_PROT_NCQ) + if (!ata_is_fpdma(qc->tf.protocol)) return ata_bmdma_qc_issue(qc); DPRINTK("Enter\n"); diff --git a/include/linux/libata.h b/include/linux/libata.h index 4e5a09c..77833f6 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -152,6 +152,7 @@ enum { ATA_PROT_FLAG_DATA = ATA_PROT_FLAG_PIO | ATA_PROT_FLAG_DMA, ATA_PROT_FLAG_NCQ = (1 << 2), /* is NCQ */ ATA_PROT_FLAG_ATAPI = (1 << 3), /* is ATAPI */ + ATA_PROT_FLAG_FPDMA = ATA_PROT_FLAG_NCQ | ATA_PROT_FLAG_DMA, /* struct ata_device stuff */ ATA_DFLAG_LBA = (1 << 0), /* device supports LBA */ @@ -1093,6 +1094,11 @@ static inline bool ata_is_data(u8 prot) return ata_prot_flags(prot) & ATA_PROT_FLAG_DATA; } +static inline bool ata_is_fpdma(u8 prot) +{ + return ata_prot_flags(prot) & ATA_PROT_FLAG_FPDMA; +} + static inline int is_multi_taskfile(struct ata_taskfile *tf) { return (tf->command == ATA_CMD_READ_MULTI) ||