| Message ID | 1425365453-19358-2-git-send-email-yanjiang.jin@windriver.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Herbert Xu |
| Headers | show |
On Tue, 3 Mar 2015 14:50:51 +0800 <yanjiang.jin@windriver.com> wrote: > This commit is to avoid the below warnings: > > drivers/crypto/caam/sg_sw_sec4.h:88:12: warning: > 'dma_map_sg_chained' defined but not used [-Wunused-function] > static int dma_map_sg_chained(struct device *dev, struct scatterlist *sg, > ^ > drivers/crypto/caam/sg_sw_sec4.h:104:12: warning: > 'dma_unmap_sg_chained' defined but not used [-Wunused-function] > static int dma_unmap_sg_chained(struct device *dev, > ^ I'm not seeing these warnings - both caamalg.c and caamhash.c use those functions fine. > -static int dma_map_sg_chained(struct device *dev, struct scatterlist *sg, > +static inline int dma_map_sg_chained(struct device *dev, struct scatterlist *sg, > unsigned int nents, enum dma_data_direction dir, > bool chained) not to mention this isn't how to fix a defined but not used warning: marking the functions inline results in different compiler output. NACK from me. Kim -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015?03?04? 02:59, Kim Phillips wrote: > On Tue, 3 Mar 2015 14:50:51 +0800 > <yanjiang.jin@windriver.com> wrote: > >> This commit is to avoid the below warnings: >> >> drivers/crypto/caam/sg_sw_sec4.h:88:12: warning: >> 'dma_map_sg_chained' defined but not used [-Wunused-function] >> static int dma_map_sg_chained(struct device *dev, struct scatterlist *sg, >> ^ >> drivers/crypto/caam/sg_sw_sec4.h:104:12: warning: >> 'dma_unmap_sg_chained' defined but not used [-Wunused-function] >> static int dma_unmap_sg_chained(struct device *dev, >> ^ > I'm not seeing these warnings - both caamalg.c and caamhash.c use > those functions fine. As you said, both caamalg.c and caamhash.c use those functions, so no warning reported. But if a new file just wants to include "sg_sw_sec4.h", doesn't want to use these functions, the above warnings will appear. We can find an example in Freescale SDK 1.6: caampkc.c includes pkc_desc.h, pkc_desc.h includes sg_sw_sec4.h, but caampkc.c doesn't call those functions. Without my patch, every file which includes sg_sw_sec4.h must call these two functions in the future, I don't think it is a good idea. Thanks! Yanjiang > >> -static int dma_map_sg_chained(struct device *dev, struct scatterlist *sg, >> +static inline int dma_map_sg_chained(struct device *dev, struct scatterlist *sg, >> unsigned int nents, enum dma_data_direction dir, >> bool chained) > not to mention this isn't how to fix a defined but not used warning: > marking the functions inline results in different compiler output. > > NACK from me. > > Kim > > -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015?03?04? 10:32, yjin wrote: > > On 2015?03?04? 02:59, Kim Phillips wrote: >> On Tue, 3 Mar 2015 14:50:51 +0800 >> <yanjiang.jin@windriver.com> wrote: >> >>> This commit is to avoid the below warnings: >>> >>> drivers/crypto/caam/sg_sw_sec4.h:88:12: warning: >>> 'dma_map_sg_chained' defined but not used [-Wunused-function] >>> static int dma_map_sg_chained(struct device *dev, struct >>> scatterlist *sg, >>> ^ >>> drivers/crypto/caam/sg_sw_sec4.h:104:12: warning: >>> 'dma_unmap_sg_chained' defined but not used [-Wunused-function] >>> static int dma_unmap_sg_chained(struct device *dev, >>> ^ >> I'm not seeing these warnings - both caamalg.c and caamhash.c use >> those functions fine. > > As you said, both caamalg.c and caamhash.c use those functions, so no > warning reported. > > But if a new file just wants to include "sg_sw_sec4.h", doesn't want > to use these functions, the above warnings will appear. > > We can find an example in Freescale SDK 1.6: > caampkc.c includes pkc_desc.h, pkc_desc.h includes sg_sw_sec4.h, but > caampkc.c doesn't call those functions. > > Without my patch, every file which includes sg_sw_sec4.h must call > these two functions in the future, I don't think it is a good idea. > > Thanks! > Yanjiang >> >>> -static int dma_map_sg_chained(struct device *dev, struct >>> scatterlist *sg, >>> +static inline int dma_map_sg_chained(struct device *dev, struct >>> scatterlist *sg, >>> unsigned int nents, enum dma_data_direction dir, >>> bool chained) >> not to mention this isn't how to fix a defined but not used warning: >> marking the functions inline results in different compiler output. >> >> NACK from me. An alternative is moving the definitions to a ".c" file, but I don't think it will be fundamental different. I know I am fixing a potential error which doesn't exist now, it seems useless for the current upstream version, we can abandon my patch. But I still think the current implementation adds unnecessary restrictions for its users. Thanks! Yanjiang >> >> Kim >> >> > > -- > To unsubscribe from this list: send the line "unsubscribe > linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > > -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/04/2015 06:57 AM, yjin wrote: > An alternative is moving the definitions to a ".c" file, but I don't > think it will be fundamental different. > I know I am fixing a potential error which doesn't exist now, it seems > useless for the current upstream version, we can abandon my patch. But I > still think the current implementation adds unnecessary restrictions for > its users. I think that both dma_map_sg_chained and dma_unmap_sg_chained should go away. They were added to support chained scatterlists, but as far as I verified, dma_map_sg should handle that case as well. Kim, can you confirm this? Cristian S. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 4 Mar 2015 11:03:28 +0200 Cristian Stoica <cristian.stoica@freescale.com> wrote: > On 03/04/2015 06:57 AM, yjin wrote: > > An alternative is moving the definitions to a ".c" file, but I don't > > think it will be fundamental different. > > I know I am fixing a potential error which doesn't exist now, it seems > > useless for the current upstream version, we can abandon my patch. But I > > still think the current implementation adds unnecessary restrictions for > > its users. > > I think that both dma_map_sg_chained and dma_unmap_sg_chained should go > away. They were added to support chained scatterlists, but as far as I > verified, dma_map_sg should handle that case as well. > > Kim, can you confirm this? I don't see how, e.g., for one, dma_map_sg is I/O TLB implementation-dependent. Kim -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/04/2015 08:34 PM, Kim Phillips wrote: > I don't see how, e.g., for one, dma_map_sg is I/O TLB > implementation-dependent. I'll need some remedial classes on this topic, but for the moment I don't see how this matters for mapping sg chains. Can you share some pointers? Thanks, Cristian S. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 5 Mar 2015 09:12:13 +0200 Cristian Stoica <cristian.stoica@freescale.com> wrote: > On 03/04/2015 08:34 PM, Kim Phillips wrote: > > > I don't see how, e.g., for one, dma_map_sg is I/O TLB > > implementation-dependent. > > I'll need some remedial classes on this topic, but for the moment I > don't see how this matters for mapping sg chains. Can you share some > pointers? I think it's better the driver not depend on whether the dma_map_sg implementation supports chains, but you're welcome to try... Kim -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" 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/crypto/caam/sg_sw_sec4.h b/drivers/crypto/caam/sg_sw_sec4.h index 3b91821..a6276eb 100644 --- a/drivers/crypto/caam/sg_sw_sec4.h +++ b/drivers/crypto/caam/sg_sw_sec4.h @@ -85,7 +85,7 @@ static inline int sg_count(struct scatterlist *sg_list, int nbytes, return sg_nents; } -static int dma_map_sg_chained(struct device *dev, struct scatterlist *sg, +static inline int dma_map_sg_chained(struct device *dev, struct scatterlist *sg, unsigned int nents, enum dma_data_direction dir, bool chained) { @@ -101,9 +101,9 @@ static int dma_map_sg_chained(struct device *dev, struct scatterlist *sg, return nents; } -static int dma_unmap_sg_chained(struct device *dev, struct scatterlist *sg, - unsigned int nents, enum dma_data_direction dir, - bool chained) +static inline int dma_unmap_sg_chained(struct device *dev, + struct scatterlist *sg, unsigned int nents, + enum dma_data_direction dir, bool chained) { if (unlikely(chained)) { int i;