| Message ID | 1369990487-23510-2-git-send-email-sw0312.kim@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 31-05-13 10:54, Seung-Woo Kim schreef: > dma-buf attachment has only exporter private data, but importer private data > can be useful for importer especially to re-import the same dma-buf. > To use importer private data in attachment of the device, the function to > search attachment in the attachment list of dma-buf is also added. > > Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com> > --- > drivers/base/dma-buf.c | 31 +++++++++++++++++++++++++++++++ > include/linux/dma-buf.h | 4 ++++ > 2 files changed, 35 insertions(+), 0 deletions(-) > > diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c > index 08fe897..a1eaaf2 100644 > --- a/drivers/base/dma-buf.c > +++ b/drivers/base/dma-buf.c > @@ -259,6 +259,37 @@ err_attach: > EXPORT_SYMBOL_GPL(dma_buf_attach); > > /** > + * dma_buf_get_attachment - Get attachment with the device from dma_buf's > + * attachments list > + * @dmabuf: [in] buffer to find device from. > + * @dev: [in] device to be found. > + * > + * Returns struct dma_buf_attachment * attaching the device; may return > + * negative error codes. > + * > + */ > +struct dma_buf_attachment *dma_buf_get_attachment(struct dma_buf *dmabuf, > + struct device *dev) > +{ > + struct dma_buf_attachment *attach; > + > + if (WARN_ON(!dmabuf || !dev)) > + return ERR_PTR(-EINVAL); > + > + mutex_lock(&dmabuf->lock); > + list_for_each_entry(attach, &dmabuf->attachments, node) { > + if (attach->dev == dev) { > + mutex_unlock(&dmabuf->lock); > + return attach; > + } > + } > + mutex_unlock(&dmabuf->lock); > + > + return ERR_PTR(-ENODEV); > +} > +EXPORT_SYMBOL_GPL(dma_buf_get_attachment); NAK in any form.. Spot the race condition between dma_buf_get_attachment and dma_buf_attach.... ~Maarten
Hello Maarten, On 2013? 06? 05? 22:23, Maarten Lankhorst wrote: > Op 31-05-13 10:54, Seung-Woo Kim schreef: >> dma-buf attachment has only exporter private data, but importer private data >> can be useful for importer especially to re-import the same dma-buf. >> To use importer private data in attachment of the device, the function to >> search attachment in the attachment list of dma-buf is also added. >> >> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com> >> --- >> drivers/base/dma-buf.c | 31 +++++++++++++++++++++++++++++++ >> include/linux/dma-buf.h | 4 ++++ >> 2 files changed, 35 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c >> index 08fe897..a1eaaf2 100644 >> --- a/drivers/base/dma-buf.c >> +++ b/drivers/base/dma-buf.c >> @@ -259,6 +259,37 @@ err_attach: >> EXPORT_SYMBOL_GPL(dma_buf_attach); >> >> /** >> + * dma_buf_get_attachment - Get attachment with the device from dma_buf's >> + * attachments list >> + * @dmabuf: [in] buffer to find device from. >> + * @dev: [in] device to be found. >> + * >> + * Returns struct dma_buf_attachment * attaching the device; may return >> + * negative error codes. >> + * >> + */ >> +struct dma_buf_attachment *dma_buf_get_attachment(struct dma_buf *dmabuf, >> + struct device *dev) >> +{ >> + struct dma_buf_attachment *attach; >> + >> + if (WARN_ON(!dmabuf || !dev)) >> + return ERR_PTR(-EINVAL); >> + >> + mutex_lock(&dmabuf->lock); >> + list_for_each_entry(attach, &dmabuf->attachments, node) { >> + if (attach->dev == dev) { >> + mutex_unlock(&dmabuf->lock); >> + return attach; >> + } >> + } >> + mutex_unlock(&dmabuf->lock); >> + >> + return ERR_PTR(-ENODEV); >> +} >> +EXPORT_SYMBOL_GPL(dma_buf_get_attachment); > NAK in any form.. > > Spot the race condition between dma_buf_get_attachment and dma_buf_attach.... Both dma_buf_get_attachment and dma_buf_attach has mutet with dmabuf->lock, and dma_buf_get_attachment is used for get attachment from same device before calling dma_buf_attach. While, dma_buf_detach can removes attachment because it does not have ref count. So importer should check ref count in its importer private data before calling dma_buf_detach if the importer want to use dma_buf_get_attachment. And dma_buf_get_attachment is for the importer, so exporter attach and detach callback operation should not call it as like exporter detach callback operation should not call dma_buf_attach if you mean this kind of race. If you have other considerations here, please describe more specifically. Thanks and Best Regards, - Seung-Woo Kim > > ~Maarten > >
Op 07-06-13 04:32, ??? schreef: > Hello Maarten, > > On 2013? 06? 05? 22:23, Maarten Lankhorst wrote: >> Op 31-05-13 10:54, Seung-Woo Kim schreef: >>> dma-buf attachment has only exporter private data, but importer private data >>> can be useful for importer especially to re-import the same dma-buf. >>> To use importer private data in attachment of the device, the function to >>> search attachment in the attachment list of dma-buf is also added. >>> >>> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com> >>> --- >>> drivers/base/dma-buf.c | 31 +++++++++++++++++++++++++++++++ >>> include/linux/dma-buf.h | 4 ++++ >>> 2 files changed, 35 insertions(+), 0 deletions(-) >>> >>> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c >>> index 08fe897..a1eaaf2 100644 >>> --- a/drivers/base/dma-buf.c >>> +++ b/drivers/base/dma-buf.c >>> @@ -259,6 +259,37 @@ err_attach: >>> EXPORT_SYMBOL_GPL(dma_buf_attach); >>> >>> /** >>> + * dma_buf_get_attachment - Get attachment with the device from dma_buf's >>> + * attachments list >>> + * @dmabuf: [in] buffer to find device from. >>> + * @dev: [in] device to be found. >>> + * >>> + * Returns struct dma_buf_attachment * attaching the device; may return >>> + * negative error codes. >>> + * >>> + */ >>> +struct dma_buf_attachment *dma_buf_get_attachment(struct dma_buf *dmabuf, >>> + struct device *dev) >>> +{ >>> + struct dma_buf_attachment *attach; >>> + >>> + if (WARN_ON(!dmabuf || !dev)) >>> + return ERR_PTR(-EINVAL); >>> + >>> + mutex_lock(&dmabuf->lock); >>> + list_for_each_entry(attach, &dmabuf->attachments, node) { >>> + if (attach->dev == dev) { >>> + mutex_unlock(&dmabuf->lock); >>> + return attach; >>> + } >>> + } >>> + mutex_unlock(&dmabuf->lock); >>> + >>> + return ERR_PTR(-ENODEV); >>> +} >>> +EXPORT_SYMBOL_GPL(dma_buf_get_attachment); >> NAK in any form.. >> >> Spot the race condition between dma_buf_get_attachment and dma_buf_attach.... > Both dma_buf_get_attachment and dma_buf_attach has mutet with > dmabuf->lock, and dma_buf_get_attachment is used for get attachment from > same device before calling dma_buf_attach. hint: what happens if 2 threads do this: if (IS_ERR(attach = dma_buf_get_attachment(buf, dev))) attach = dma_buf_attach(buf, dev); There really is no correct usecase for this kind of thing, so please don't do it. > While, dma_buf_detach can removes attachment because it does not have > ref count. So importer should check ref count in its importer private > data before calling dma_buf_detach if the importer want to use > dma_buf_get_attachment. > > And dma_buf_get_attachment is for the importer, so exporter attach and > detach callback operation should not call it as like exporter detach > callback operation should not call dma_buf_attach if you mean this kind > of race. > > If you have other considerations here, please describe more specifically. > > Thanks and Best Regards, > - Seung-Woo Kim > >> ~Maarten >> >>
On 2013? 06? 07? 20:24, Maarten Lankhorst wrote: > Op 07-06-13 04:32, ??? schreef: >> Hello Maarten, >> >> On 2013? 06? 05? 22:23, Maarten Lankhorst wrote: >>> Op 31-05-13 10:54, Seung-Woo Kim schreef: >>>> dma-buf attachment has only exporter private data, but importer private data >>>> can be useful for importer especially to re-import the same dma-buf. >>>> To use importer private data in attachment of the device, the function to >>>> search attachment in the attachment list of dma-buf is also added. >>>> >>>> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com> >>>> --- >>>> drivers/base/dma-buf.c | 31 +++++++++++++++++++++++++++++++ >>>> include/linux/dma-buf.h | 4 ++++ >>>> 2 files changed, 35 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c >>>> index 08fe897..a1eaaf2 100644 >>>> --- a/drivers/base/dma-buf.c >>>> +++ b/drivers/base/dma-buf.c >>>> @@ -259,6 +259,37 @@ err_attach: >>>> EXPORT_SYMBOL_GPL(dma_buf_attach); >>>> >>>> /** >>>> + * dma_buf_get_attachment - Get attachment with the device from dma_buf's >>>> + * attachments list >>>> + * @dmabuf: [in] buffer to find device from. >>>> + * @dev: [in] device to be found. >>>> + * >>>> + * Returns struct dma_buf_attachment * attaching the device; may return >>>> + * negative error codes. >>>> + * >>>> + */ >>>> +struct dma_buf_attachment *dma_buf_get_attachment(struct dma_buf *dmabuf, >>>> + struct device *dev) >>>> +{ >>>> + struct dma_buf_attachment *attach; >>>> + >>>> + if (WARN_ON(!dmabuf || !dev)) >>>> + return ERR_PTR(-EINVAL); >>>> + >>>> + mutex_lock(&dmabuf->lock); >>>> + list_for_each_entry(attach, &dmabuf->attachments, node) { >>>> + if (attach->dev == dev) { >>>> + mutex_unlock(&dmabuf->lock); >>>> + return attach; >>>> + } >>>> + } >>>> + mutex_unlock(&dmabuf->lock); >>>> + >>>> + return ERR_PTR(-ENODEV); >>>> +} >>>> +EXPORT_SYMBOL_GPL(dma_buf_get_attachment); >>> NAK in any form.. >>> >>> Spot the race condition between dma_buf_get_attachment and dma_buf_attach.... >> Both dma_buf_get_attachment and dma_buf_attach has mutet with >> dmabuf->lock, and dma_buf_get_attachment is used for get attachment from >> same device before calling dma_buf_attach. > > hint: what happens if 2 threads do this: > > if (IS_ERR(attach = dma_buf_get_attachment(buf, dev))) > attach = dma_buf_attach(buf, dev); > > There really is no correct usecase for this kind of thing, so please don't do it. Ok, dma_buf_get_attachment can not prevent attachments from one device. Anyway, do you think that importer side private data, not to allow re-import one dma-buf to a device, has some advantage? If that, I'll add some check_and_attach function, otherwise, I'll find other way. Thanks and Regards, - Seung-Woo Kim > >> While, dma_buf_detach can removes attachment because it does not have >> ref count. So importer should check ref count in its importer private >> data before calling dma_buf_detach if the importer want to use >> dma_buf_get_attachment. >> >> And dma_buf_get_attachment is for the importer, so exporter attach and >> detach callback operation should not call it as like exporter detach >> callback operation should not call dma_buf_attach if you mean this kind >> of race. >> >> If you have other considerations here, please describe more specifically. >> >> Thanks and Best Regards, >> - Seung-Woo Kim >> >>> ~Maarten >>> >>> > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel >
diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c index 08fe897..a1eaaf2 100644 --- a/drivers/base/dma-buf.c +++ b/drivers/base/dma-buf.c @@ -259,6 +259,37 @@ err_attach: EXPORT_SYMBOL_GPL(dma_buf_attach); /** + * dma_buf_get_attachment - Get attachment with the device from dma_buf's + * attachments list + * @dmabuf: [in] buffer to find device from. + * @dev: [in] device to be found. + * + * Returns struct dma_buf_attachment * attaching the device; may return + * negative error codes. + * + */ +struct dma_buf_attachment *dma_buf_get_attachment(struct dma_buf *dmabuf, + struct device *dev) +{ + struct dma_buf_attachment *attach; + + if (WARN_ON(!dmabuf || !dev)) + return ERR_PTR(-EINVAL); + + mutex_lock(&dmabuf->lock); + list_for_each_entry(attach, &dmabuf->attachments, node) { + if (attach->dev == dev) { + mutex_unlock(&dmabuf->lock); + return attach; + } + } + mutex_unlock(&dmabuf->lock); + + return ERR_PTR(-ENODEV); +} +EXPORT_SYMBOL_GPL(dma_buf_get_attachment); + +/** * dma_buf_detach - Remove the given attachment from dmabuf's attachments list; * optionally calls detach() of dma_buf_ops for device-specific detach * @dmabuf: [in] buffer to detach from. diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index dfac5ed..09cff0f 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -136,6 +136,7 @@ struct dma_buf { * @dev: device attached to the buffer. * @node: list of dma_buf_attachment. * @priv: exporter specific attachment data. + * @importer_priv: importer specific attachment data. * * This structure holds the attachment information between the dma_buf buffer * and its user device(s). The list contains one attachment struct per device @@ -146,6 +147,7 @@ struct dma_buf_attachment { struct device *dev; struct list_head node; void *priv; + void *importer_priv; }; /** @@ -164,6 +166,8 @@ static inline void get_dma_buf(struct dma_buf *dmabuf) struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, struct device *dev); +struct dma_buf_attachment *dma_buf_get_attachment(struct dma_buf *dmabuf, + struct device *dev); void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *dmabuf_attach);
dma-buf attachment has only exporter private data, but importer private data can be useful for importer especially to re-import the same dma-buf. To use importer private data in attachment of the device, the function to search attachment in the attachment list of dma-buf is also added. Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com> --- drivers/base/dma-buf.c | 31 +++++++++++++++++++++++++++++++ include/linux/dma-buf.h | 4 ++++ 2 files changed, 35 insertions(+), 0 deletions(-)