Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20180219140739.24383-1-khandual@linux.vnet.ibm.com>
References: <20180219140739.24383-1-khandual@linux.vnet.ibm.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Mon, 19 Feb 2018 16:49:19 +0200
Message-ID: <CAHp75VeUB_ZB2-7NNK7WCQ62HrSe2WVarS8Bcm1mpyBoVd2FqA@mail.gmail.com>
Subject: Re: [PATCH V3] lib/scatterlist: Add SG_CHAIN and SG_LAST macros for
 LSB encodings
To:     Anshuman Khandual <khandual@linux.vnet.ibm.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Bart Van Assche <bart.vanassche@wdc.com>,
        Johannes Thumshirn <jthumshirn@suse.de>,
        Jens Axboe <axboe@kernel.dk>,
        Chris Wilson <chris@chris-wilson.co.uk>,
        Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Feb 19, 2018 at 4:07 PM, Anshuman Khandual
<khandual@linux.vnet.ibm.com> wrote:
> This replaces scatterlist->page_link LSB encodings with SG_CHAIN and
> SG_LAST definitions without any functional change. While here also
> add macro definitions SG_PAGE_BITS and SG_PAGE_MASK.
>

A nit below, in any case

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
> ---
> Changes in V3:
> - Changed SG_END as SG_LAST due to a build failure
> - Added SG_PAGE_BITS and SG_PAGE_MASK as per Tvrtko
> - Used BIT() macro as per Chris
>
> 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 | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
> index 22b2131bcdcd..494b84c7f3c8 100644
> --- a/include/linux/scatterlist.h
> +++ b/include/linux/scatterlist.h
> @@ -66,15 +66,20 @@ struct sg_table {
>
>  #define SG_MAGIC       0x87654321
>
> +#define SG_CHAIN       BIT(0)
> +#define SG_LAST                BIT(1)
> +#define SG_PAGE_BITS   (SG_CHAIN | SG_LAST)
> +#define SG_PAGE_MASK   (~SG_PAGE_BITS)
> +
>  /*
>   * 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_LAST)
>  #define sg_chain_ptr(sg)       \
> -       ((struct scatterlist *) ((sg)->page_link & ~0x03))
> +       ((struct scatterlist *) ((sg)->page_link & SG_PAGE_MASK))
>
>  /**
>   * sg_assign_page - Assign a given page to an SG entry
> @@ -88,13 +93,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_PAGE_BITS;
>
>         /*
>          * 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_PAGE_BITS);
>  #ifdef CONFIG_DEBUG_SG
>         BUG_ON(sg->sg_magic != SG_MAGIC);
>         BUG_ON(sg_is_chain(sg));
> @@ -130,7 +135,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_PAGE_MASK);
>  }
>
>  /**
> @@ -178,7 +183,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_LAST;

If it's ~2-3 characters over 80, I would rather still keep on the same line.

>  }
>
>  /**
> @@ -198,8 +204,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_LAST;
> +       sg->page_link &= ~SG_CHAIN;
>  }
>
>  /**
> @@ -215,7 +221,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_LAST;
>  }
>
>  /**
> --
> 2.11.0
>


-- 
With Best Regards,
Andy Shevchenko