linux-kernel - Re: [PATCH 4/5] ide: Add support for TRIM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <200904022137.35671.bzolnier@gmail.com>
Date:	Thu, 2 Apr 2009 21:37:35 +0200
From:	Bartlomiej Zolnierkiewicz <bzolnier@...il.com>
To:	Matthew Wilcox <willy@...ux.intel.com>
Cc:	Sergei Shtylyov <sshtylyov@...mvista.com>,
	linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org,
	jgarzik@...hat.com, David Woodhouse <David.Woodhouse@...el.com>
Subject: Re: [PATCH 4/5] ide: Add support for TRIM

On Thursday 02 April 2009, Matthew Wilcox wrote:
> On Thu, Apr 02, 2009 at 08:38:03PM +0400, Sergei Shtylyov wrote:
> > Hello.
> >>>> +	task->tf_flags   = IDE_TFLAG_LBA48 | IDE_TFLAG_OUT_HOB |
> >>>> +			   IDE_TFLAG_OUT_TF | IDE_TFLAG_OUT_DEVICE |
> >
> >>>   The last 3 flags are going to be obsoleted too...
> >
> >> So if I remove them today, the command will still work?
> >
> >     s/obsoleted/renamed and moved to another other field/ -- I'm going to 
> > submit a patchset refactoring 'struct ide_cmd and 'struct ide-taskfile' 
> > at last...
> 
> Since I'm coding to today's kernel, not to a patch which doesn't exist
> yet, taking them out won't work very well.
> 
> I've not been paying much attention to drivers/ide, so I've no idea
> whether the following patch works.  It does at least compile:
> 
> +++ b/drivers/ide/ide-disk.c
> @@ -407,7 +407,7 @@ static void idedisk_prepare_flush(struct request_queue *q, s
>  static int idedisk_prepare_discard(struct request_queue *q, struct request *rq,
>                                                         struct bio *bio)
>  {
> -       ide_task_t *task;
> +       struct ide_cmd *task;
>         unsigned size;
>         struct page *page = alloc_page(GFP_KERNEL);
>         if (!page)
> @@ -432,8 +432,8 @@ static int idedisk_prepare_discard(struct request_queue *q, 
>  
>         task->tf_flags   = IDE_TFLAG_LBA48 | IDE_TFLAG_OUT_HOB |
>                            IDE_TFLAG_OUT_TF | IDE_TFLAG_OUT_DEVICE |
> -                          IDE_TFLAG_DYN;
> -       task->data_phase = TASKFILE_OUT_DMA;
> +                          IDE_TFLAG_DYN | IDE_TFLAG_WRITE;
> +       task->protocol = ATA_PROT_DMA;
>  
>         rq->cmd_type = REQ_TYPE_ATA_TASKFILE;
>         rq->special = task;
> 
> If I've understood 0dfb991c6943c810175376b58d1c29cfe532541b correctly,
> this should be equivalent.  Bart?

Yep.  The final patch looks fine overall (thanks for adding TRIM support
also to drivers/ide), with the small exception for this chunk:

@@ -521,6 +567,9 @@ static int set_wcache(ide_drive_t *drive, int arg)
 
        update_ordered(drive);
 
+       if (ata_id_has_trim(drive->id))
+               blk_queue_set_discard(drive->queue, idedisk_prepare_discard);
+
        return err;
 }

as it would fit better somewhere in ide_disk_setup() ISO set_wcache()...

[ When it comes to CodingStyle issues I don't care that much though I think
  that applying Sergei's suggestions will make the patch more consistent with
  the rest of drivers/ide code... ]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/