This replaces scatterlist->page_link LSB encodings with SG_CHAIN and
SG_END definitions without any functional change.
Signed-off-by: Anshuman Khandual <[email protected]>
---
Changes in V2:
- Changed SG_EMARK as SG_END as per Johannes and Tvrtko
- Added 'UL' to the constants as per Bart
include/linux/scatterlist.h | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 22b2131bcdcd..b6fe1815f5c4 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -65,16 +65,18 @@ struct sg_table {
*/
#define SG_MAGIC 0x87654321
+#define SG_CHAIN 0x01UL
+#define SG_END 0x02UL
/*
* We overload the LSB of the page pointer to indicate whether it's
* a valid sg entry, or whether it points to the start of a new scatterlist.
* Those low bits are there for everyone! (thanks mason :-)
*/
-#define sg_is_chain(sg) ((sg)->page_link & 0x01)
-#define sg_is_last(sg) ((sg)->page_link & 0x02)
+#define sg_is_chain(sg) ((sg)->page_link & SG_CHAIN)
+#define sg_is_last(sg) ((sg)->page_link & SG_END)
#define sg_chain_ptr(sg) \
- ((struct scatterlist *) ((sg)->page_link & ~0x03))
+ ((struct scatterlist *) ((sg)->page_link & ~(SG_CHAIN | SG_END)))
/**
* sg_assign_page - Assign a given page to an SG entry
@@ -88,13 +90,13 @@ struct sg_table {
**/
static inline void sg_assign_page(struct scatterlist *sg, struct page *page)
{
- unsigned long page_link = sg->page_link & 0x3;
+ unsigned long page_link = sg->page_link & (SG_CHAIN | SG_END);
/*
* In order for the low bit stealing approach to work, pages
* must be aligned at a 32-bit boundary as a minimum.
*/
- BUG_ON((unsigned long) page & 0x03);
+ BUG_ON((unsigned long) page & (SG_CHAIN | SG_END));
#ifdef CONFIG_DEBUG_SG
BUG_ON(sg->sg_magic != SG_MAGIC);
BUG_ON(sg_is_chain(sg));
@@ -130,7 +132,7 @@ static inline struct page *sg_page(struct scatterlist *sg)
BUG_ON(sg->sg_magic != SG_MAGIC);
BUG_ON(sg_is_chain(sg));
#endif
- return (struct page *)((sg)->page_link & ~0x3);
+ return (struct page *)((sg)->page_link & ~(SG_CHAIN | SG_END));
}
/**
@@ -178,7 +180,8 @@ static inline void sg_chain(struct scatterlist *prv, unsigned int prv_nents,
* Set lowest bit to indicate a link pointer, and make sure to clear
* the termination bit if it happens to be set.
*/
- prv[prv_nents - 1].page_link = ((unsigned long) sgl | 0x01) & ~0x02;
+ prv[prv_nents - 1].page_link = ((unsigned long) sgl | SG_CHAIN)
+ & ~SG_END;
}
/**
@@ -198,8 +201,8 @@ static inline void sg_mark_end(struct scatterlist *sg)
/*
* Set termination bit, clear potential chain bit
*/
- sg->page_link |= 0x02;
- sg->page_link &= ~0x01;
+ sg->page_link |= SG_END;
+ sg->page_link &= ~SG_CHAIN;
}
/**
@@ -215,7 +218,7 @@ static inline void sg_unmark_end(struct scatterlist *sg)
#ifdef CONFIG_DEBUG_SG
BUG_ON(sg->sg_magic != SG_MAGIC);
#endif
- sg->page_link &= ~0x02;
+ sg->page_link &= ~SG_END;
}
/**
--
2.11.0
On 2/14/18 8:33 PM, Anshuman Khandual wrote:
> This replaces scatterlist->page_link LSB encodings with SG_CHAIN and
> SG_END definitions without any functional change.
Looks good to me, I'll add it for 4.17.
--
Jens Axboe
Quoting Anshuman Khandual (2018-02-15 03:33:56)
> This replaces scatterlist->page_link LSB encodings with SG_CHAIN and
> SG_END definitions without any functional change.
>
> Signed-off-by: Anshuman Khandual <[email protected]>
> ---
> Changes in V2:
> - Changed SG_EMARK as SG_END as per Johannes and Tvrtko
> - Added 'UL' to the constants as per Bart
>
> include/linux/scatterlist.h | 23 +++++++++++++----------
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
> index 22b2131bcdcd..b6fe1815f5c4 100644
> --- a/include/linux/scatterlist.h
> +++ b/include/linux/scatterlist.h
> @@ -65,16 +65,18 @@ struct sg_table {
> */
>
> #define SG_MAGIC 0x87654321
> +#define SG_CHAIN 0x01UL
> +#define SG_END 0x02UL
The alternative would be BIT(0) and BIT(1), which by the time you write
the constant out in full, as above, become clearer.
I think you also want whitespace between the MAGIC and CHAIN,END so that
the reader doesn't think they refer to some magic encoding of SG_MAGIC.
-Chris
Hi Anshuman,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v4.16-rc1 next-20180216]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Anshuman-Khandual/lib-scatterlist-Add-SG_CHAIN-and-SG_END-macros-for-LSB-encodings/20180218-015554
config: i386-randconfig-s0-201807 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
In file included from drivers/misc/cardreader/rtsx_pcr.c:32:0:
>> include/linux/rtsx_pci.h:40:0: warning: "SG_END" redefined
#define SG_END 0x02
In file included from include/linux/dmapool.h:14:0,
from include/linux/pci.h:1316,
from drivers/misc/cardreader/rtsx_pcr.c:22:
include/linux/scatterlist.h:69:0: note: this is the location of the previous definition
#define SG_END 0x02UL
vim +/SG_END +40 include/linux/rtsx_pci.h
67d16a46 include/linux/mfd/rtsx_pci.h Wei WANG 2012-11-09 37
ada71f55 include/linux/mfd/rtsx_pci.h Micky Ching 2015-02-25 38 #define RTSX_HDBAR 0x08
ada71f55 include/linux/mfd/rtsx_pci.h Micky Ching 2015-02-25 39 #define SG_INT 0x04
ada71f55 include/linux/mfd/rtsx_pci.h Micky Ching 2015-02-25 @40 #define SG_END 0x02
ada71f55 include/linux/mfd/rtsx_pci.h Micky Ching 2015-02-25 41 #define SG_VALID 0x01
ada71f55 include/linux/mfd/rtsx_pci.h Micky Ching 2015-02-25 42 #define SG_NO_OP 0x00
ada71f55 include/linux/mfd/rtsx_pci.h Micky Ching 2015-02-25 43 #define SG_TRANS_DATA (0x02 << 4)
ada71f55 include/linux/mfd/rtsx_pci.h Micky Ching 2015-02-25 44 #define SG_LINK_DESC (0x03 << 4)
ada71f55 include/linux/mfd/rtsx_pci.h Micky Ching 2015-02-25 45 #define RTSX_HDBCTLR 0x0C
67d16a46 include/linux/mfd/rtsx_pci.h Wei WANG 2012-11-09 46 #define SDMA_MODE 0x00
67d16a46 include/linux/mfd/rtsx_pci.h Wei WANG 2012-11-09 47 #define ADMA_MODE (0x02 << 26)
67d16a46 include/linux/mfd/rtsx_pci.h Wei WANG 2012-11-09 48 #define STOP_DMA (0x01 << 28)
67d16a46 include/linux/mfd/rtsx_pci.h Wei WANG 2012-11-09 49 #define TRIG_DMA (0x01 << 31)
67d16a46 include/linux/mfd/rtsx_pci.h Wei WANG 2012-11-09 50
:::::: The code at line 40 was first introduced by commit
:::::: ada71f5588320e7a5c7166cb7c1c8c40cb866ac4 mfd: rtsx: Place register address and values togather
:::::: TO: Micky Ching <[email protected]>
:::::: CC: Lee Jones <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Anshuman,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v4.16-rc1 next-20180216]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Anshuman-Khandual/lib-scatterlist-Add-SG_CHAIN-and-SG_END-macros-for-LSB-encodings/20180218-015554
config: x86_64-randconfig-s3-02180201 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
In file included from drivers/staging/rts5208/rtsx.h:180:0,
from drivers/staging/rts5208/rtsx.c:28:
>> drivers/staging/rts5208/rtsx_chip.h:343:0: warning: "SG_END" redefined
#define SG_END 0x02
In file included from include/linux/blkdev.h:28:0,
from drivers/staging/rts5208/rtsx.c:23:
include/linux/scatterlist.h:69:0: note: this is the location of the previous definition
#define SG_END 0x02UL
vim +/SG_END +343 drivers/staging/rts5208/rtsx_chip.h
fa590c22 Micky Ching 2013-11-12 340
fa590c22 Micky Ching 2013-11-12 341 /* SG descriptor */
fa590c22 Micky Ching 2013-11-12 342 #define SG_INT 0x04
fa590c22 Micky Ching 2013-11-12 @343 #define SG_END 0x02
fa590c22 Micky Ching 2013-11-12 344 #define SG_VALID 0x01
fa590c22 Micky Ching 2013-11-12 345
:::::: The code at line 343 was first introduced by commit
:::::: fa590c222fbaa428edb2ce2194638906cea1400a staging: rts5208: add support for rts5208 and rts5288
:::::: TO: Micky Ching <[email protected]>
:::::: CC: Greg Kroah-Hartman <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 02/18/2018 12:58 AM, kbuild test robot wrote:
> Hi Anshuman,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on linus/master]
> [also build test WARNING on v4.16-rc1 next-20180216]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/Anshuman-Khandual/lib-scatterlist-Add-SG_CHAIN-and-SG_END-macros-for-LSB-encodings/20180218-015554
> config: x86_64-randconfig-s3-02180201 (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=x86_64
>
> All warnings (new ones prefixed by >>):
>
> In file included from drivers/staging/rts5208/rtsx.h:180:0,
> from drivers/staging/rts5208/rtsx.c:28:
>>> drivers/staging/rts5208/rtsx_chip.h:343:0: warning: "SG_END" redefined
> #define SG_END 0x02
>
> In file included from include/linux/blkdev.h:28:0,
> from drivers/staging/rts5208/rtsx.c:23:
> include/linux/scatterlist.h:69:0: note: this is the location of the previous definition
> #define SG_END 0x02UL
>
>
> vim +/SG_END +343 drivers/staging/rts5208/rtsx_chip.h
SG_END is already defined in a driver. Hence shall we change back
'SG_END' definition as 'SG_EMARK' or any other suggestions ?
On Mon, Feb 19, 2018 at 09:42:29AM +0530, Anshuman Khandual wrote:
> SG_END is already defined in a driver. Hence shall we change back
> 'SG_END' definition as 'SG_EMARK' or any other suggestions ?
Yes something like this.
--
Johannes Thumshirn Storage
[email protected] +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
On 19/02/18 04:12, Anshuman Khandual wrote:
> On 02/18/2018 12:58 AM, kbuild test robot wrote:
>> Hi Anshuman,
>>
>> Thank you for the patch! Perhaps something to improve:
>>
>> [auto build test WARNING on linus/master]
>> [also build test WARNING on v4.16-rc1 next-20180216]
>> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>>
>> url: https://github.com/0day-ci/linux/commits/Anshuman-Khandual/lib-scatterlist-Add-SG_CHAIN-and-SG_END-macros-for-LSB-encodings/20180218-015554
>> config: x86_64-randconfig-s3-02180201 (attached as .config)
>> compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
>> reproduce:
>> # save the attached .config to linux build tree
>> make ARCH=x86_64
>>
>> All warnings (new ones prefixed by >>):
>>
>> In file included from drivers/staging/rts5208/rtsx.h:180:0,
>> from drivers/staging/rts5208/rtsx.c:28:
>>>> drivers/staging/rts5208/rtsx_chip.h:343:0: warning: "SG_END" redefined
>> #define SG_END 0x02
>>
>> In file included from include/linux/blkdev.h:28:0,
>> from drivers/staging/rts5208/rtsx.c:23:
>> include/linux/scatterlist.h:69:0: note: this is the location of the previous definition
>> #define SG_END 0x02UL
>>
>>
>> vim +/SG_END +343 drivers/staging/rts5208/rtsx_chip.h
>
> SG_END is already defined in a driver. Hence shall we change back
> 'SG_END' definition as 'SG_EMARK' or any other suggestions ?
Maybe SG_LAST? Going by existence of sg_is_chain and sg_is_last.
And also would probably be good to add something like:
#define SG_PAGE_BITS (SG_CHAIN | SG_LAST)
#define SG_PAGE_MASK (~SG_PAGE_BITS)
?
Regards,
Tvrtko
On Mon, Feb 19, 2018 at 12:10 PM, Tvrtko Ursulin <[email protected]> wrote:
> On 19/02/18 04:12, Anshuman Khandual wrote:
>> On 02/18/2018 12:58 AM, kbuild test robot wrote:
>>> #define SG_END 0x02
>>> In file included from include/linux/blkdev.h:28:0,
>>> from drivers/staging/rts5208/rtsx.c:23:
>>> include/linux/scatterlist.h:69:0: note: this is the location of the
>>> previous definition
>>> #define SG_END 0x02UL
>>>
>>> vim +/SG_END +343 drivers/staging/rts5208/rtsx_chip.h
>>
>>
>> SG_END is already defined in a driver.
In some driver which is by the way staging one.
> Hence shall we change back
>> 'SG_END' definition as 'SG_EMARK' or any other suggestions ?
>
>
> Maybe SG_LAST? Going by existence of sg_is_chain and sg_is_last.
The mentioned driver should be cleaned up as well (and perhaps to start with).
It allows us to rectify constants in global namespace.
So, from my point of view the driver code is ugly in a sense it
doesn't have its own namespace for constants.
After we fix the driver, the rest would be whatever we get agreed on.
> And also would probably be good to add something like:
>
> #define SG_PAGE_BITS (SG_CHAIN | SG_LAST)
> #define SG_PAGE_MASK (~SG_PAGE_BITS)
>
> ?
Good suggestion,
--
With Best Regards,
Andy Shevchenko